Closed Bug 1261265 Opened 4 years ago Closed 4 years ago

668941.xhtml and download-3-notref.html are going to permafail on Android debug when Gecko 48 merges to Aurora (Assertion failure: (mParent->mBits & mask) == (aNewParent->mBits & mask), at layout/style/nsStyleContext.cpp:330)

Categories

(Core :: CSS Parsing and Computation, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 + fixed
firefox48 + fixed

People

(Reporter: RyanVM, Assigned: heycam)

References

Details

(Keywords: assertion, crash)

Attachments

(1 file)

[Tracking Requested - why for this release]: Android debug permafail when Gecko 48 merges to Aurora.

https://treeherder.mozilla.org/logviewer.html#?job_id=18726915&repo=try
Flags: needinfo?(dbaron)
Summary: 668941.xhtml is going to permafail on Android debug when Gecko 48 merges to Aurora (Assertion failure: (mParent->mBits & mask) == (aNewParent->mBits & mask), at layout/style/nsStyleContext.cpp:330) → 668941.xhtml and download-3-notref.html are going to permafail on Android debug when Gecko 48 merges to Aurora (Assertion failure: (mParent->mBits & mask) == (aNewParent->mBits & mask), at layout/style/nsStyleContext.cpp:330)
Getting low on time in this release cycle. Do you have cycles to look into this, Cam?
Flags: needinfo?(cam)
I don't know why this would fail on a merge.  I'll have a look.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
Ryan do you know how I can get output included in the logs?  I am trying to use printf_stderr to dump out more information about the assertion but I'm not seeing it.

https://hg.mozilla.org/try/file/f018f07fab7b5a4fb5de5afe6c5340eca78d9a5b/layout/style/nsStyleContext.cpp#l330
https://treeherder.mozilla.org/logviewer.html#?job_id=19126368&repo=try
Flags: needinfo?(ryanvm)
printf_stderr ends up in logcat, which is captured separately & linked from the fancy treeherder log page.

Direct link to that log: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/904a5220d16f66e6ea65b00ea84fcd97f0fcabd3813b930eecec6edbe6a2bbcbf6dfa1f3d0e6cceb53644165bf10fd15466a31390b5cc8c806ada6d99f37907d

That log contains the following "!=" information just before the assertion, which I think might be the output you're looking for, based on your try run's printf_stderr tweak:
{
04-06 22:22:33.416  1085  1103 I Gecko   : 2000000 != 0
04-06 22:22:33.426  1085  1103 F MOZ_Assert: Assertion failure: (mParent->mBits & mask) == (aNewParent->mBits & mask), at /builds/slave/try-and-api-15-d-0000000000000/build/src/layout/style/nsStyleContext.cpp:333
}
Flags: needinfo?(ryanvm)
(In reply to Cameron McCormack (:heycam) from comment #3)
> https://treeherder.mozilla.org/logviewer.html#?job_id=19126368&repo=try

(The logcat log w/ printf_stderr output is linked as "logcat-emulator-5554.log" on this ^^ page, BTW.)
Ah, thank you Daniel.
The assertions we have at the top of nsStyleContext::MoveTo check that we aren't moving a style context between parents whose NS_STYLE_HAS_PSEUDO_ELEMENT_DATA or NS_STYLE_IN_DISPLAY_NONE_SUBTREE bits differ, since that would require us to set/clear those bits on descendants.  Checks up in RestyleManager should prevent us from stopping restyling (and calling nsStyleContext::MoveTo) when we detect cases like this that would need us to traverse down the tree restyling because of changes to these bits.

We already check that the bits have the same value on the the style context itself (and its putative new style context, which we discard when we detect all style values are the same), so we don't need to worry about the bits on this style context.  But it's actually fine to call MoveTo if the bits differ on parents -- as long as the bit is set on the child.  In this case, the change in parent bit value can't affect the bit value on this style context and its descendants.

So I think we want to changes these assertions to check MoveTo is only called (a) when the bits on the parents are the same, or (b) when the bit on this style context is set.
Flags: needinfo?(dbaron)
And in fact we already do this for the NS_STYLE_HAS_TEXT_DECORATION_LINES and NS_STYLE_RELEVANT_LINK_VISITED checks, just not for NS_STYLE_HAS_PSEUDO_ELEMENT_DATA and NS_STYLE_IN_DISPLAY_NONE_SUBTREE.
Attached patch patchSplinter Review
Attachment #8739944 - Flags: review?(dholbert)
Comment on attachment 8739944 [details] [diff] [review]
patch

Review of attachment 8739944 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, several wording nits:


> Bug 1261265 - Fix nsStyleContext::MoveTo flag assertions. r?dholbert

Consider making the commit message slightly more descriptive, e.g. adding at the end: "...to allow mismatch on the parents if bit is set on child".

::: layout/style/nsStyleContext.cpp
@@ +347,5 @@
>  
>    // This function shouldn't be getting called if the parents have different
> +  // values for some flags in mBits, unless the flag is set on this style
> +  // context, because if that were the case we would need/ to recompute those
> +  // bits for |this|.

Two nits to clarify this comment:
 (1) s/is set/is also set/   (I think?)
 (2) Put the whole "unless...context" phrase in parenthesis.

Nit #2 is important, because with the current phrasing, it's ambiguous which scenario you're referring to when you say "if that were the case" -- right now it can very easily be *misread* as referring to the "unless the flag is set on this style context" scenario.  If you separate that part out out using parens, then it's more clear that "if that were the case" really refers to the scenario discussed at the beginning of the sentence (and the parenthesized chunk is merely a clarification).
Attachment #8739944 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/a578a48aae83
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.