1329919 - [Stylo] crash while loading cnn.com snapshot

Assignee

Description

•

7 years ago

Attached file cnn.com index.html — Details

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

Cameron McCormack (:heycam)

Comment 1

•

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

Shing Lyu [:shinglyu]

Assignee

Updated

•

7 years ago

Blocks: 1330550

Shing Lyu [:shinglyu]

Assignee

Comment 2

•

7 years ago

The reduced test case is simply:

`<a href="index.html" style="display: none;" />` in a file called `index.html`

Shing Lyu [:shinglyu]

Assignee

Comment 3

•

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

Shing Lyu [:shinglyu]

Assignee

Comment 4

•

7 years ago

Fixed the "tp" test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e07d71e29d7f67b7a16be4c1082a5c52725eac9

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/109956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109956/

Cameron McCormack (:heycam)

Comment 6

•

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

Comment on attachment 8833835 [details]
Bug 1329919 - Skip repaint frame hint if there is no frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109956/diff/1-2/

Shing Lyu [:shinglyu]

Assignee

Comment 8

•

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

Cameron McCormack (:heycam)

Comment 9

•

7 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+

Shing Lyu [:shinglyu]

Assignee

Updated

•

7 years ago

Assignee: nobody → slyu

Comment hidden (mozreview-request)

Comment on attachment 8833835 [details]
Bug 1329919 - Skip repaint frame hint if there is no frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109956/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8833835 [details]
Bug 1329919 - Skip repaint frame hint if there is no frame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109956/diff/3-4/

Pulsebot

Comment 12

•

7 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

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 13

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/8e075d90bf31

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox54: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla54

cnn.com index.html 7 years ago Shing Lyu [:shinglyu] 98.18 KB, text/html		Details
Bug 1329919 - Skip repaint frame hint if there is no frame. 7 years ago Shing Lyu [:shinglyu] 59 bytes, text/x-review-board-request	heycam : review+	Details

Bugzilla

Quick Search

[Stylo] crash while loading cnn.com snapshot

Categories

(Core :: CSS Parsing and Computation, defect)

Tracking

()

People

(Reporter: shinglyu, Assigned: shinglyu)

References

Details

Crash Data

Security

(public)

User Story

Attachments

(2 files)

Description

Comment 1

Updated

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Updated

Comment 10

Comment 11

Comment 12

Comment 13

Attachment

General

Description

File Name

Content Type