Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAssertion failure when logging into gmail #14480
Closed
Labels
Comments
|
cc @emilio |
|
This assertion was added by @bholley in https://github.com/servo/servo/blame/e11441bdbce9c5bf2a1ba45faf938bab994f387c/components/script/dom/node.rs#L245. |
|
Hm. We're supposed to have the invariant that a node never has the dirty descendants bit (or non-null layout data) when it's not in a document. I don't have the cycles to debug this, but could make good piranha food? |
|
I agree that the fix could end up being trivial, but diagnosing it may not. I bet it could be something like what was causing the underflows in #10110? That is, an element kind that Gmail uses where our implementation of |
|
Or maybe an unbind_from_tree impl that sets dirty bits around? That'd be weird, but I guess it can happen transitively. I'll take a quick look though I'm about to sleep. I can't promise anything though. |
|
This also reproduces on http://universidad.continental.edu.pe. |
|
After throwing a bunch of assertions in places that set the flags, I finally caught this. The flag is being set on elements that aren't actually in the document during layout; next step is figuring out why layout is interacting with nodes that aren't in the document. |
|
Heh, #14776 |
|
The easy answer is: |
|
(or |
|
That is in the backtrace of the script thread, so that's plausible! |
|
However, CSSStyleDeclaration::get_computed_style checks whether the node is in a document, so it's got to be more subtle than that. |
|
@jdm do you have that backtrace? Other suspects are |
|
|
Full backtrace: |
|
My assertions: diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs
index d2bdf3a..9a95c9a 100644
--- a/components/layout_thread/lib.rs
+++ b/components/layout_thread/lib.rs
@@ -1082,6 +1082,8 @@ impl LayoutThread {
let mut next = iter.next();
while let Some(node) = next {
+ use script::dom::node::LayoutNodeHelpers;
+ assert!(node.is_in_doc());
if node.needs_dirty_on_viewport_size_changed() {
let el = node.as_element().unwrap();
el.mutate_data().map(|mut d| d.restyle()
diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs
index 3e2de9b..f0e00c7 100644
--- a/components/script/dom/node.rs
+++ b/components/script/dom/node.rs
@@ -290,6 +290,7 @@ impl Node {
self.owner_doc().content_and_heritage_changed(self, NodeDamage::OtherNodeDamage);
child.owner_doc().content_and_heritage_changed(child, NodeDamage::OtherNodeDamage);
+ assert!(!child.has_dirty_descendants());
}
pub fn to_untrusted_node_address(&self) -> UntrustedNodeAddress {
@@ -419,6 +420,9 @@ impl Node {
flags.remove(flag);
}
+ if value && flag.contains(HAS_DIRTY_DESCENDANTS) {
+ assert!(self.is_in_doc());
+ }
self.flags.set(flags);
}
@@ -427,6 +431,7 @@ impl Node {
}
pub fn set_has_dirty_descendants(&self, state: bool) {
+ assert!(self.is_in_doc());
self.set_flag(HAS_DIRTY_DESCENDANTS, state)
}
diff --git a/components/script/layout_wrapper.rs b/components/script/layout_wrapper.rs
index dca0750..659d1c4 100644
--- a/components/script/layout_wrapper.rs
+++ b/components/script/layout_wrapper.rs
@@ -36,7 +36,7 @@ use dom::bindings::js::LayoutJS;
use dom::characterdata::LayoutCharacterDataHelpers;
use dom::document::{Document, LayoutDocumentHelpers, PendingRestyle};
use dom::element::{Element, LayoutElementHelpers, RawLayoutElementHelpers};
-use dom::node::{CAN_BE_FRAGMENTED, DIRTY_ON_VIEWPORT_SIZE_CHANGE, HAS_DIRTY_DESCENDANTS};
+use dom::node::{CAN_BE_FRAGMENTED, DIRTY_ON_VIEWPORT_SIZE_CHANGE, HAS_DIRTY_DESCENDANTS, IS_IN_DOC};
use dom::node::{LayoutNodeHelpers, Node};
use dom::text::Text;
use gfx_traits::ByteIndex;
@@ -131,6 +131,12 @@ impl<'ln> ServoLayoutNode<'ln> {
pub fn as_document(&self) -> Option<ServoLayoutDocument<'ln>> {
self.node.downcast().map(ServoLayoutDocument::from_layout_js)
}
+
+ pub fn is_in_doc(&self) -> bool {
+ unsafe {
+ self.node.get_flag(IS_IN_DOC)
+ }
+ }
}
impl<'ln> NodeInfo for ServoLayoutNode<'ln> {
@@ -400,6 +406,8 @@ impl<'le> TElement for ServoLayoutElement<'le> {
}
unsafe fn set_dirty_descendants(&self) {
+ assert!(self.as_node().node.get_flag(IS_IN_DOC));
+
self.as_node().node.set_flag(HAS_DIRTY_DESCENDANTS, true)
}It's hitting the one added to |
|
I think I see one problem - the pending restyle vector in Document can contain nodes that have been removed from the tree, since they're never removed from the vector except by layout. |
diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs
index 79acb69..4a2b998 100644
--- a/components/script/dom/document.rs
+++ b/components/script/dom/document.rs
@@ -67,7 +67,7 @@ use dom::keyboardevent::KeyboardEvent;
use dom::location::Location;
use dom::messageevent::MessageEvent;
use dom::mouseevent::MouseEvent;
-use dom::node::{self, CloneChildrenFlag, Node, NodeDamage, window_from_node};
+use dom::node::{self, CloneChildrenFlag, Node, NodeDamage, window_from_node, IS_IN_DOC, LayoutNodeHelpers};
use dom::nodeiterator::NodeIterator;
use dom::nodelist::NodeList;
use dom::pagetransitionevent::PageTransitionEvent;
@@ -1790,7 +1790,7 @@ impl LayoutDocumentHelpers for LayoutJS<Document> {
#[allow(unrooted_must_root)]
unsafe fn drain_pending_restyles(&self) -> Vec<(LayoutJS<Element>, PendingRestyle)> {
let mut elements = (*self.unsafe_get()).pending_restyles.borrow_mut_for_layout();
- let result = elements.drain().map(|(k, v)| (k.to_layout(), v)).collect();
+ let result = elements.drain().map(|(k, v)| (k.to_layout(), v)).filter(|&(ref k, _)| k.upcast::<Node>().get_flag(IS_IN_DOC)).collect();
result
}
diff --git a/components/script/layout_wrapper.rs b/components/script/layout_wrapper.rs
index e6c38aa..b1bde4e 100644
--- a/components/script/layout_wrapper.rs
+++ b/components/script/layout_wrapper.rs
@@ -36,7 +36,7 @@ use dom::bindings::js::LayoutJS;
use dom::characterdata::LayoutCharacterDataHelpers;
use dom::document::{Document, LayoutDocumentHelpers, PendingRestyle};
use dom::element::{Element, LayoutElementHelpers, RawLayoutElementHelpers};
-use dom::node::{CAN_BE_FRAGMENTED, DIRTY_ON_VIEWPORT_SIZE_CHANGE, HAS_DIRTY_DESCENDANTS};
+use dom::node::{CAN_BE_FRAGMENTED, DIRTY_ON_VIEWPORT_SIZE_CHANGE, HAS_DIRTY_DESCENDANTS, IS_IN_DOC};
use dom::node::{LayoutNodeHelpers, Node};
use dom::text::Text;
use gfx_traits::ByteIndex;
@@ -400,6 +400,7 @@ impl<'le> TElement for ServoLayoutElement<'le> {
}
unsafe fn set_dirty_descendants(&self) {
+ assert!(self.as_node().node.get_flag(IS_IN_DOC));
self.as_node().node.set_flag(HAS_DIRTY_DESCENDANTS, true)
}This patch appears to fix the problem for me. |
|
Hmm... Good catch. I guess constructing a test case isn't terribly hard. That patch looks ok to me (with a comment in the |
This was referenced Jan 1, 2017
bors-servo
added a commit
that referenced
this issue
Jan 4, 2017
Avoid restyling elements that aren't in a document - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14480 - [X] There are tests for these changes <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14845) <!-- Reviewable:end -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
STR: