Closed Bug 1390179 Opened 2 years ago Closed 2 years ago

Stylo: Crash on Kaspersky forum

Categories

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

57 Branch
Unspecified
All
defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- verified
firefox57 --- verified

People

(Reporter: ajfhajf, Assigned: bholley)

References

(Blocks 1 open bug)

Details

(Keywords: crash, nightly-community, reproducible)

Crash Data

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170814100258

Steps to reproduce:

Create new profile, enable Stylo in about:config, close Nightly, launch Nightly
Go to 
https://forum.kaspersky.com/index.php?/topic/373421-%D1%80%D0%B0%D0%B7%D0%B3%D0%BE%D0%B2%D0%BE%D1%80%D1%8B-%D0%B7%D0%B0-%D0%B6%D0%B8%D0%B7%D0%BD%D1%8C/&page=13
Click on 14 in order to go to page 14



Actual results:

In ~5 seconds tab will crash



Expected results:

No crash. Bug is not reproduced if Stylo is disabled.
I have same result, crash, with same steps. 
crash report https://crash-stats.mozilla.com/report/index/0f7db9bb-c535-4481-b26c-7e0b70170814
Ubuntu 17.04 64bit. Stylo is enabled.
Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170813183258
On crash i got this in terminal 
"thread '<unnamed>' panicked at 'Resolving style on unstyled element', /checkout/src/libcore/option.rs:823"
I can also reproduce on Windows10 with same crash id bp-ca910cbb-39f2-41de-ac04-58a370170814.
https://hg.mozilla.org/mozilla-central/rev/df9beb781895fcd0493c21e95ad313e0044515ec
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID 20170814100258
Status: UNCONFIRMED → NEW
Crash Signature: [@ core::option::expect_failed | geckoservo::glue::Servo_ResolveStyle]
Has STR: --- → yes
Component: Untriaged → CSS Parsing and Computation
Ever confirmed: true
Keywords: crash, reproducible
Product: Firefox → Core
Priority: -- → P1
Thank you!
Confirmed in Nightly 57 x64 20170814100258 @ Debian Testing.
bp-de576621-70c3-4ec6-8938-c46b20170814
Crash Signature: [@ core::option::expect_failed | geckoservo::glue::Servo_ResolveStyle] → [@ core::option::expect_failed | geckoservo::glue::Servo_ResolveStyle] [@ mozalloc_abort | abort | core::option::expect_failed | geckoservo::glue::Servo_ResolveStyle ]
OS: Unspecified → All
Hiro, is this crash related to FlushThrottledStyles bug 1388031?
Flags: needinfo?(hikezoe)
See Also: → 1388031
(In reply to Chris Peterson [:cpeterson] from comment #5)
> Hiro, is this crash related to FlushThrottledStyles bug 1388031?

I don't think so.

Anyway, I just caught this in rr. Shouldn't be long now.
Assignee: nobody → bobbyholley
Flags: needinfo?(hikezoe)
So this turned out to be rather tricky to debug, even with rr. Mostly because the DOM is enormous and the pathology is pretty indirect.

At a very high level, the bug is a stale ANCESTOR_WAS_RECONSTRUCTED bit in the tree.

This happens because various edge cases can cause us to propagate that bit to a child without setting dirty descendants on the parent. In turn, this means that the post-traversal doesn't find the bit and clear it (since ClearRestyleStateFromSubtree relies on the descendant bits).

Once that bit is there, it causes problems later, when we propagate it down a subtree, and then restyle something to display:none. That causes us to avoid computing a change hint for the display:none transition (since we think an ancestor was reconstructed), which means that we don't handle it in the post-traversal and leave the frame attached to the display:none element.

This, in turn, means that inserting a child of the display:none element causes us to propagate up the lazy frame construction bit (normally we wouldn't because we wouldn't have a parent frame). This causes us to try to construct frames for the unstyled content, which blows up.

Constructing a crashtest for all this would be rather finicky and probably not very valuable. The robust fix is just to update all the restyle bits on every traversal (like we do for TRAVERSED_WITHOUT_STYLING already).
This fixes the crash.

MozReview-Commit-ID: 76q5XxK2o2a
Attachment #8897202 - Flags: review?(emilio+bugs)
That commit had some garbage in it.

MozReview-Commit-ID: 76q5XxK2o2a
Attachment #8897203 - Flags: review?(emilio+bugs)
Attachment #8897202 - Attachment is obsolete: true
Attachment #8897202 - Flags: review?(emilio+bugs)
MozReview-Commit-ID: 9KQVOpdEjwF
Attachment #8897207 - Flags: review?(emilio+bugs)
Comment on attachment 8897203 [details] [diff] [review]
Avoid leaving stale ANCESTOR_WAS_RECONSTRUCTED bits in the tree. v1

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

I still think a test-case would be nice, fwiw.
Attachment #8897203 - Flags: review?(emilio+bugs) → review+
Attachment #8897207 - Flags: review?(emilio+bugs) → review+
Does this crash also affect Stylo in Beta 56?
(In reply to Chris Peterson [:cpeterson] from comment #13)
> Does this crash also affect Stylo in Beta 56?
Yes.
bp-36f63efc-d9b9-4514-87e5-3b2ae0170815
Blocks: 1390610
https://hg.mozilla.org/integration/autoland/rev/5aa91b77b3a939dbe24e6d9abd12bbd83c226f6c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Please request Beta approval on this when you get a chance. Though I'm not entirely sure if a straight-up graft is what we want here given that it's patching upstream Servo code. If a branch-specific patch is warranted, please attach that as well :)
Flags: needinfo?(bobbyholley)
Target Milestone: --- → mozilla57
(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #4)
> Thank you!
> Confirmed in Nightly 57 x64 20170814100258 @ Debian Testing.
> bp-de576621-70c3-4ec6-8938-c46b20170814

Verified fixed in Nightly 57 x64 20170816100153 @ Debian Testing.
(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Please request Beta approval on this when you get a chance.

Will do.

> Though I'm not
> entirely sure if a straight-up graft is what we want here given that it's
> patching upstream Servo code. If a branch-specific patch is warranted,
> please attach that as well :)

Patching code in servo/ on beta is just as easy as patching code in layout/. The tricky part is patching the vendored dependencies in third_party/rust/, which we don't have here.
Flags: needinfo?(bobbyholley)
Comment on attachment 8897203 [details] [diff] [review]
Avoid leaving stale ANCESTOR_WAS_RECONSTRUCTED bits in the tree. v1

Request applies to both patches in the bug I guess. We don't really need the bonus fix, but it's small and that's what landed on Nightly, so it's worth keeping them consistent.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Crashes for our stylo experiment on beta 56.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes (comment 19).
[Needs manual test from QE? If yes, steps to reproduce]: Yes, STR in comment 0. Though maybe just NI darkspirit to verify.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only affects stylo, very simple fix.
[String changes made/needed]: None
Attachment #8897203 - Flags: approval-mozilla-beta?
Comment on attachment 8897203 [details] [diff] [review]
Avoid leaving stale ANCESTOR_WAS_RECONSTRUCTED bits in the tree. v1

Fix a stylo crash and was verified. Beta56+.
Attachment #8897203 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This crash signature is continuing in high volume in the 8-17 Nightly, and I'd have thought that it would have made it to Nightly by now. I don't know if you want to reopen this or file a new bug (it seems like a fairly generic signature?). Here's an example: bp-7df39974-bb2e-48e5-a28f-9ef670170817
Flags: needinfo?(bobbyholley)
I just went and filed another bug. Feel free to dupe it over here if appropriate.
Flags: needinfo?(bobbyholley)
Flags: qe-verify+
Verified in comment 19.
Status: RESOLVED → VERIFIED
(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #14)
> (In reply to Chris Peterson [:cpeterson] from comment #13)
> > Does this crash also affect Stylo in Beta 56?
> Yes.
> bp-36f63efc-d9b9-4514-87e5-3b2ae0170815

Seems fine now. Beta 56.0b4 x64 20170819205558 @ Debian Testing
I managed to reproduce this issue using Nightly from 2017-08-14 on Windows 10 x64, Ubuntu 16.04 x64 and mac OS 10.12.

I retested everything using Beta 56.0b7 and latest Nightly 57.0a1 across platforms and the bug is not reproducible anymore.
You need to log in before you can comment on or make changes to this bug.