Closed
Bug 1329919
Opened 8 years ago
Closed 8 years ago
[Stylo] crash while loading cnn.com snapshot
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: shinglyu, Assigned: shinglyu)
References
Details
Attachments
(2 files)
Got this crash while loading cnn.com from tp5 test suite
Log:
------------------------------------------------------------
./mach run --disable-e10s ~/Downloads/index.html
0:00.12 /home/shinglyu/workspace/stylo/stylo-incubator-2/obj-x86_64-pc-linux-gnu/dist/bin/firefox /home/shinglyu/Downloads/index.html -no-remote -profile /home/shinglyu/workspace/stylo/stylo-incubator-2/obj-x86_64-pc-linux-gnu/tmp/scratch_user
ERROR:geckoservo::glue: Unnecessary call to traverse_subtree
ERROR:geckoservo::glue: Unnecessary call to traverse_subtree
ERROR:geckoservo::glue: Unnecessary call to traverse_subtree
ERROR:geckoservo::glue: Unnecessary call to traverse_subtree
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
stylo: skipping declaration without ParserContextExtraData
ERROR:geckoservo::glue: Unnecessary call to traverse_subtree
ERROR:geckoservo::glue: Unnecessary call to traverse_subtree
ERROR:geckoservo::glue: Unnecessary call to traverse_subtree
ERROR:geckoservo::glue: Unnecessary call to traverse_subtree
Comment 1•8 years ago
|
||
I don't get a crash with that attachment, but I guess it relies on some external style sheets. I downloaded tp5n.zip and ran a debug build on the cnn.com/www.cnn.com/index.html in there and got this:
Assertion failure: aFrame || (aHint & nsChangeHint_ReconstructFrame) (must have frame), at /z/stylo/incubator/layout/base/nsStyleChangeList.cpp:20
#0 0x00007fffe6eb3eca in nsStyleChangeList::AppendChange (this=0x7fffffffb198, aFrame=0x0, aContent=0x7fffbbb45fc0, aHint=nsChangeHint_RepaintFrame) at /z/stylo/incubator/layout/base/nsStyleChangeList.cpp:19
#1 0x00007fffe6e012d4 in mozilla::ServoRestyleManager::RecreateStyleContexts (this=0x7fffb6f10a00, aElement=0x7fffbbb45fc0, aParentContext=0x7fffc56a5138, aStyleSet=0x7fffb71f6550, aChangeListToProcess=...)
at /z/stylo/incubator/layout/base/ServoRestyleManager.cpp:151
#2 0x00007fffe6e018f7 in mozilla::ServoRestyleManager::RecreateStyleContexts (this=0x7fffb6f10a00, aElement=0x7fffc2757b30, aParentContext=0x7fffc56a44c0, aStyleSet=0x7fffb71f6550, aChangeListToProcess=...)
at /z/stylo/incubator/layout/base/ServoRestyleManager.cpp:234
#3 0x00007fffe6e018f7 in mozilla::ServoRestyleManager::RecreateStyleContexts (this=0x7fffb6f10a00, aElement=0x7fffc2757aa0, aParentContext=0x7fffb879cef0, aStyleSet=0x7fffb71f6550, aChangeListToProcess=...)
at /z/stylo/incubator/layout/base/ServoRestyleManager.cpp:234
#4 0x00007fffe6e018f7 in mozilla::ServoRestyleManager::RecreateStyleContexts (this=0x7fffb6f10a00, aElement=0x7fffb196a480, aParentContext=0x7fffb879cc08, aStyleSet=0x7fffb71f6550, aChangeListToProcess=...)
at /z/stylo/incubator/layout/base/ServoRestyleManager.cpp:234
#5 0x00007fffe6e018f7 in mozilla::ServoRestyleManager::RecreateStyleContexts (this=0x7fffb6f10a00, aElement=0x7fffb404c240, aParentContext=0x0, aStyleSet=0x7fffb71f6550, aChangeListToProcess=...)
at /z/stylo/incubator/layout/base/ServoRestyleManager.cpp:234
#6 0x00007fffe6e01e0e in mozilla::ServoRestyleManager::ProcessPendingRestyles (this=0x7fffb6f10a00) at /z/stylo/incubator/layout/base/ServoRestyleManager.cpp:335
#7 0x00007fffe6e09b4d in mozilla::RestyleManagerHandle::Ptr::ProcessPendingRestyles (this=0x7fffffffb350) at /z/stylo/incubator/obj/dist/include/mozilla/RestyleManagerHandleInlines.h:75
#8 0x00007fffe6dd4cd3 in mozilla::PresShell::FlushPendingNotifications (this=0x7fffb719d400, aFlush=...) at /z/stylo/incubator/layout/base/PresShell.cpp:4096
#9 0x00007fffe6dd46c8 in mozilla::PresShell::FlushPendingNotifications (this=0x7fffb719d400, aType=mozilla::FlushType::Layout) at /z/stylo/incubator/layout/base/PresShell.cpp:3986
#10 0x00007fffe4761bcf in nsDocument::FlushPendingNotifications (this=0x7fffb6f2d000, aType=mozilla::FlushType::Layout) at /z/stylo/incubator/dom/base/nsDocument.cpp:7812
#11 0x00007fffe4761a7a in nsDocument::FlushPendingNotifications (this=0x7fffbd8a2000, aType=mozilla::FlushType::Style) at /z/stylo/incubator/dom/base/nsDocument.cpp:7790
#12 0x00007fffe3dc91e1 in nsDocLoader::DocLoaderIsEmpty (this=0x7fffb41f6800, aFlushLayout=true) at /z/stylo/incubator/uriloader/base/nsDocLoader.cpp:683
#13 0x00007fffe3dca30c in nsDocLoader::OnStopRequest (this=0x7fffb41f6800, aRequest=0x7fffc1d2eba0, aCtxt=0x0, aStatus=nsresult::NS_OK) at /z/stylo/incubator/uriloader/base/nsDocLoader.cpp:612
#14 0x00007fffe2cae5df in mozilla::net::nsLoadGroup::RemoveRequest (this=0x7fffb6f7e120, request=0x7fffc1d2eba0, ctxt=0x0, aStatus=nsresult::NS_OK) at /z/stylo/incubator/netwerk/base/nsLoadGroup.cpp:633
#15 0x00007fffe4764c70 in nsDocument::DoUnblockOnload (this=0x7fffbd8a2000) at /z/stylo/incubator/dom/base/nsDocument.cpp:8664
#16 0x00007fffe4764a2d in nsDocument::UnblockOnload (this=0x7fffbd8a2000, aFireSync=true) at /z/stylo/incubator/dom/base/nsDocument.cpp:8592
...
Assignee | ||
Comment 2•8 years ago
|
||
The reduced test case is simply:
`<a href="index.html" style="display: none;" />` in a file called `index.html`
Assignee | ||
Comment 3•8 years ago
|
||
I've made a patch, talos is still running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eba8fbf74bd64153752a97357354626a994e9af0
(We also have a lot of unexpected reftest behaviors, I might address that in another bug.
Assignee | ||
Comment 4•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8833835 [details]
Bug 1329919 - Skip repaint frame hint if there is no frame.
https://reviewboard.mozilla.org/r/109956/#review112222
::: layout/base/ServoRestyleManager.cpp:152
(Diff revision 1)
> nsIFrame* primaryFrame = aElement->GetPrimaryFrame();
>
> // FIXME(bholley): Once we transfer ownership of the styles to the frame, we
> // can fast-reject without the FFI call by checking mServoData for null.
> nsChangeHint changeHint = Servo_TakeChangeHint(aElement);
> + if (primaryFrame) { // If there is no frame we should skip this
Ah, I think we need to continue to append if there is a ReconstructFrame hint in there, since that is the one case where we are allowed to append a change hint when there is no frame, and processing this change hint is the way that we recreate a frame for an element that goes from display:none to something else. (It doesn't matter if there are other hints along with the ReconstructFrame, as RestyleManagerBase::ProcessRestyledFrames will ignore them and just do the frame reconstruction work.)
After that change, let's expand this comment a bit to say that this is for posted restyle hints when there is no frame. So maybe something like:
// Although we shouldn't generate non-ReconstructFrame hints for elements with
// no frames, we can still get them here if they were explicitly posted by
// PostRestyleEvent, such as a RepaintFrame hint when a :link changes to be
// :visited. Skip processing these hints if there is no frame.
Attachment #8833835 -
Flags: review?(cam) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8833835 [details]
Bug 1329919 - Skip repaint frame hint if there is no frame.
I tested it with
<span id="foo" style="display:none">FOOBAR</span>
<script type="text/javascript" charset="utf-8">
setTimeout(function(){
document.getElementById("foo").style.display = "block";
}, 1000)
</script>
Seems to be correct.
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8833835 [details]
Bug 1329919 - Skip repaint frame hint if there is no frame.
https://reviewboard.mozilla.org/r/109956/#review112298
::: layout/base/ServoRestyleManager.cpp:156
(Diff revision 2)
> + if (primaryFrame || changeHint & nsChangeHint_ReconstructFrame) {
> - if (changeHint & ~nsChangeHint_NeutralChange) {
> + if (changeHint & ~nsChangeHint_NeutralChange) {
May as well combine the two if statements.
Attachment #8833835 -
Flags: review?(cam) → review+
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → slyu
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•8 years ago
|
||
Pushed by slyu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e075d90bf31
Skip repaint frame hint if there is no frame. r=heycam
Comment 13•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•