Closed Bug 1413361 Opened 7 years ago Closed 7 years ago

thread panicked at Resolving style on <html>

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla58

Tracking Flags:

Tracking

Status

firefox-esr52

---

unaffected

firefox56

---

unaffected

firefox57

---

wontfix

firefox58

---

fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 1 obsolete file)

test_case.html 7 years ago Tyson Smith [:tsmith] 253 bytes, text/html		Details
log.txt 7 years ago Tyson Smith [:tsmith] 11.19 KB, text/plain		Details
Bug 1413361: ResolveStyleFor should not flush stuff. 7 years ago Emilio Cobos Álvarez (:emilio) 59 bytes, text/x-review-board-request		Details
Bug 1413361: EnsureFrameForTextNode shouldn't reconstruct synchronously without up-to-date styles. 7 years ago Emilio Cobos Álvarez (:emilio) 59 bytes, text/x-review-board-request	bzbarsky : review+	Details

Tyson Smith [:tsmith]

Reporter

Description

•

7 years ago

Attached file test_case.html — Details

thread '<unnamed>' panicked at 'Resolving style on <html> (0x60e00024d620) without current styles: ElementData { styles: ElementStyles { primary: Some(Some(StrongRuleNode { p: 0x607000440ac0 })), pseudos: EagerPseudoStyles(None) }, damage: GeckoRestyleDamage(nsChangeHint(0)), hint: RESTYLE_SELF | RESTYLE_DESCENDANTS, flags: (empty) }', /src/servo/ports/geckolib/glue.rs:3460:4
stack backtrace:
   0:     0x7f4035f5c163 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::hfc7985b08e763a82
                               at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1:     0x7f4035f569a4 - std::sys_common::backtrace::_print::h16a1db02a59ead63
                               at /checkout/src/libstd/sys_common/backtrace.rs:71
   2:     0x7f4035f69883 - std::panicking::default_hook::{{closure}}::h48ecee46f2eefc30
                               at /checkout/src/libstd/sys_common/backtrace.rs:60
                               at /checkout/src/libstd/panicking.rs:381
   3:     0x7f4035f695f2 - std::panicking::default_hook::hb4c92ae8d005ca44
                               at /checkout/src/libstd/panicking.rs:397
   4:     0x7f4035f69d47 - std::panicking::rust_panic_with_hook::h25d461655d60b1a5
                               at /checkout/src/libstd/panicking.rs:611
   5:     0x7f4035f69ba4 - std::panicking::begin_panic::h0f6fdd9abfd7dfb9
                               at /checkout/src/libstd/panicking.rs:572
   6:     0x7f4035f69b19 - std::panicking::begin_panic_fmt::ha31e26b280c9e878
                               at /checkout/src/libstd/panicking.rs:522
   7:     0x7f40357ce4a7 - Servo_ResolveStyle
                               at /src/servo/ports/geckolib/glue.rs:3460
   8:     0x7f4030df1727 - _ZN7mozilla13ServoStyleSet17ResolveServoStyleEPNS_3dom7ElementE
                               at /src/layout/style/ServoStyleSet.cpp:1362

Flags: in-testsuite?

Tyson Smith [:tsmith]

Reporter

Comment 1

•

7 years ago

Attached file log.txt — Details

Emilio Cobos Álvarez (:emilio)

Assignee

Updated

•

7 years ago

Flags: needinfo?(emilio)

Comment hidden (mozreview-request)

To keep the semantics closer between the two LazyComputeBehavior variants. We do
assumptions about the stylist and all that stuff being flushed in the other
variant, so we can and should do the same here.

In particular, the restyle hint here comes from resolving the mapped
declarations on the body, and then sync-construct the world from
EnsureFrameForTextNode.

That doesn't mean that sync-constructing the world without flushing doesn't
break invariants, fix for that in a second.

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

Comment hidden (mozreview-request)

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

Emilio Cobos Álvarez (:emilio)

Assignee

Updated

•

7 years ago

Assignee: nobody → emilio

Flags: needinfo?(emilio)

Boris Zbarsky [:bzbarsky]

Comment 4

•

7 years ago

mozreview-review

Comment on attachment 8924134 [details]
Bug 1413361: ResolveStyleFor should not flush stuff.

https://reviewboard.mozilla.org/r/195388/#review200560

I don't understand the semantics of this stuff well enough to competently review this absent either better documentation or a better commit message.  :(

In particular, the semantics _I_ understood for LazyComputeBehavior::Allow are "we have no idea what the state of the style system looks like, but we need up-to-date style, so update that state as needed".

In practice, most of the callers that use LazyComputeBehavior::Allow seem to be in frame code, so maybe they don't care about "really" getting up-to-date styles (and in fact can't, because flushing style from frame code is not safe).  I don't know about the nsComputedDOMStyle::ResolveWithAnimation callsite.

::: commit-message-52e4f:7
(Diff revision 1)
> +
> +To keep the semantics closer between the two LazyComputeBehavior variants. We do
> +assumptions about the stylist and all that stuff being flushed in the other
> +variant, so we can and should do the same here.
> +
> +In particular, the restyle hint here comes from resolving the mapped

What restyle hint?  Where "here"?

Attachment #8924134 - Flags: review?(bzbarsky)

Boris Zbarsky [:bzbarsky]

Comment 5

•

7 years ago

mozreview-review

Comment on attachment 8924135 [details]
Bug 1413361: EnsureFrameForTextNode shouldn't reconstruct synchronously without up-to-date styles.

https://reviewboard.mozilla.org/r/195390/#review200568

::: layout/base/nsCSSFrameConstructor.h:321
(Diff revision 1)
>    void CharacterDataChanged(nsIContent* aContent,
>                              CharacterDataChangeInfo* aInfo);
>  
>    // If aContent is a text node that has been optimized away due to being
> -  // whitespace next to a block boundary (or for some other reason), stop
> -  // doing that and create a frame for it if it should have one. This recreates
> +  // whitespace next to a block boundary (or for some other reason), ensure that
> +  // a frame for it is created if it should have one in the next flush.

Maybe "is created the next time frames are flushed, if it can possibly have a frame at all"?

Right now it sounds like the call will create the frame "if it should have one in the next flush", which is not what it does.

Attachment #8924135 - Flags: review?(bzbarsky) → review+

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 6

•

7 years ago

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> In practice, most of the callers that use LazyComputeBehavior::Allow seem to
> be in frame code, so maybe they don't care about "really" getting up-to-date
> styles (and in fact can't, because flushing style from frame code is not
> safe).  I don't know about the nsComputedDOMStyle::ResolveWithAnimation
> callsite.

Right.

Note that we don't do a style flush per se (we update the user font set, the stylist, etc), but we apparently rely on flushing stylesheets on that viewport scrollbar override call in order to get the proper scrollbars the first time, which is annoying and we should probably fix (I'll file):

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dd1cefe8f242bc41b774234b448836a621f29a7&selectedJob=141327543

> ::: commit-message-52e4f:7
> (Diff revision 1)
> > +
> > +To keep the semantics closer between the two LazyComputeBehavior variants. We do
> > +assumptions about the stylist and all that stuff being flushed in the other
> > +variant, so we can and should do the same here.
> > +
> > +In particular, the restyle hint here comes from resolving the mapped
> 
> What restyle hint?  Where "here"?

Sorry, should've commented with more detail. Here we panic because something has posted a restyle for the document element from ResolveMappedAttrDeclarationBlocks.

Ryan VanderMeulen [:RyanVM]

Comment 7

•

7 years ago

INFO: Last good revision: e16dba457260675669a0e81863849563b31a9ba2
INFO: First bad revision: 86b793bcbcd090a4189814f14204a2e0ea7929ef
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e16dba457260675669a0e81863849563b31a9ba2&tochange=86b793bcbcd090a4189814f14204a2e0ea7929ef

Blocks: 1383332

Has Regression Range: --- → yes

status-firefox56: --- → unaffected

status-firefox57: --- → wontfix

status-firefox-esr52: --- → unaffected

Version: 58 Branch → 57 Branch

Comment hidden (mozreview-request)

Comment on attachment 8924135 [details]
Bug 1413361: EnsureFrameForTextNode shouldn't reconstruct synchronously without up-to-date styles.

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

Emilio Cobos Álvarez (:emilio)

Assignee

Updated

•

7 years ago

Attachment #8924134 - Attachment is obsolete: true

Emilio Cobos Álvarez (:emilio)

Assignee

Comment 9

•

7 years ago

mozreview-review-reply

Comment on attachment 8924135 [details]
Bug 1413361: EnsureFrameForTextNode shouldn't reconstruct synchronously without up-to-date styles.

https://reviewboard.mozilla.org/r/195390/#review200568

> Maybe "is created the next time frames are flushed, if it can possibly have a frame at all"?
> 
> Right now it sounds like the call will create the frame "if it should have one in the next flush", which is not what it does.

Much better :)

Comment hidden (mozreview-request)

Comment on attachment 8924135 [details]
Bug 1413361: EnsureFrameForTextNode shouldn't reconstruct synchronously without up-to-date styles.

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

Mozilla Autoland

Comment 11

•

7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0f4ad53b4c90 -d b022ff43f4bb: rebasing 431561:0f4ad53b4c90 "Bug 1413361: EnsureFrameForTextNode shouldn't reconstruct synchronously without up-to-date styles. r=bz" (tip)
merging layout/base/nsCSSFrameConstructor.cpp
merging layout/style/crashtests/crashtests.list
warning: conflicts while merging layout/style/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment hidden (mozreview-request)

Comment on attachment 8924135 [details]
Bug 1413361: EnsureFrameForTextNode shouldn't reconstruct synchronously without up-to-date styles.

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

Pulsebot

Comment 13

•

7 years ago

Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/043ae7cc7d2e
EnsureFrameForTextNode shouldn't reconstruct synchronously without up-to-date styles. r=bz

Emilio Cobos Álvarez (:emilio)

Assignee

Updated

•

7 years ago

Comment 14

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/043ae7cc7d2e

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox58: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla58

You need to log in before you can comment on or make changes to this bug.