1261265 - 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)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

[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

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Getting low on time in this release cycle. Do you have cycles to look into this, Cam?

Comment 2

[Cameron McCormack (:heycam)]

I don't know why this would fail on a merge.  I'll have a look.

Comment 3

[Cameron McCormack (:heycam)]

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

Comment 4

[Daniel Holbert [:dholbert]]

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
}

Comment 5

[Daniel Holbert [:dholbert]]

(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.)

Comment 6

[Cameron McCormack (:heycam)]

Ah, thank you Daniel.

Comment 7

[Cameron McCormack (:heycam)]

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.

Comment 8

[Cameron McCormack (:heycam)]

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.

Comment 9

[Cameron McCormack (:heycam)]

As Aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=940459170fcb&group_state=expanded

On mozilla-central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ad26f5f34bf&group_state=expanded

Comment 10

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 11

[Daniel Holbert [: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).

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a578a48aae83

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a578a48aae83