Closed
Bug 1390179
Opened 7 years ago
Closed 7 years ago
Stylo: Crash on Kaspersky forum
Categories
(Core :: CSS Parsing and Computation, defect, P1)
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)
(deleted),
patch
|
emilio
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•7 years ago
|
||
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"
Comment 3•7 years ago
|
||
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
status-firefox57:
--- → affected
Component: Untriaged → CSS Parsing and Computation
Ever confirmed: true
Keywords: crash,
reproducible
Product: Firefox → Core
Assignee | ||
Updated•7 years ago
|
Priority: -- → P1
Comment 4•7 years ago
|
||
Thank you!
Confirmed in Nightly 57 x64 20170814100258 @ Debian Testing.
bp-de576621-70c3-4ec6-8938-c46b20170814
Blocks: stylo-site-issues
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 ]
Keywords: nightly-community
OS: Unspecified → All
Comment 5•7 years ago
|
||
Hiro, is this crash related to FlushThrottledStyles bug 1388031?
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 6•7 years ago
|
||
(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)
Assignee | ||
Comment 7•7 years ago
|
||
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).
Assignee | ||
Comment 8•7 years ago
|
||
This fixes the crash.
MozReview-Commit-ID: 76q5XxK2o2a
Attachment #8897202 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 9•7 years ago
|
||
That commit had some garbage in it.
MozReview-Commit-ID: 76q5XxK2o2a
Attachment #8897203 -
Flags: review?(emilio+bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8897202 -
Attachment is obsolete: true
Attachment #8897202 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 10•7 years ago
|
||
MozReview-Commit-ID: 9KQVOpdEjwF
Attachment #8897207 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 11•7 years ago
|
||
Comment 12•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8897207 -
Flags: review?(emilio+bugs) → review+
Comment 13•7 years ago
|
||
Does this crash also affect Stylo in Beta 56?
Comment 14•7 years ago
|
||
(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
Assignee | ||
Comment 15•7 years ago
|
||
Assignee | ||
Comment 16•7 years ago
|
||
Assignee | ||
Comment 17•7 years ago
|
||
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 18•7 years ago
|
||
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 :)
Comment 19•7 years ago
|
||
(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.
Assignee | ||
Comment 20•7 years ago
|
||
(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)
Assignee | ||
Comment 21•7 years ago
|
||
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 22•7 years ago
|
||
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+
Comment 23•7 years ago
|
||
bugherder uplift |
Comment 24•7 years ago
|
||
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)
Updated•7 years ago
|
Comment 25•7 years ago
|
||
I just went and filed another bug. Feel free to dupe it over here if appropriate.
Flags: needinfo?(bobbyholley)
Updated•7 years ago
|
Flags: qe-verify+
Comment 27•7 years ago
|
||
(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
Comment 28•7 years ago
|
||
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.
Description
•