1437790 - stylo-chrome: panicked at 'Resolving style on <image class="toolbarbutton-icon"> without current styles' on test_bug437844.xul

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

thread '<unnamed>' panicked at 'Resolving style on <image class="toolbarbutton-icon"> (0x17ccc123940) without current styles: ElementData { styles: ElementStyles { primary: Some(Some(StrongRuleNode { p: 0x17cce2af2e0 })), pseudos: EagerPseudoStyles(None) }, damage: GeckoRestyleDamage(nsChangeHint(0)), hint: RESTYLE_SELF, flags: (empty) }'

Comment 1

[Emilio Cobos Álvarez (:emilio)]

So I took a look at this, and the restyle hint comes from the document state invalidation. Now, we need to figure out why that element wasn't processed during the restyle...

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Oh, and the answer is easy, there's never a restyle...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1437790: No need to reframe synchronously on chrome-flush-skin-caches.

Instead, just post the reframe, and process them normally on the next style
flush.

The assertion happens because the observer is triggered due to a doc ltr -> rtl
change, which triggers style invalidation. There's nothing processing the
restyles resulting from that invalidation before we hackily go into frame
construction from there, and thus we assert that the styles aren't up-to-date.

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

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8950519 [details]
Bug 1437790: No need to reframe synchronously on chrome-flush-skin-caches.

https://reviewboard.mozilla.org/r/219798/#review225818

So the reframe was initially introduced in bug 252703 to workaround theme switch issue. It sounds like nothing really depends on a synchronous reframe, so posting a restyle event to ensure the frames get reframed eventually should be enough.

r=me with the comment addressed.

::: layout/base/PresShell.cpp:9271
(Diff revision 1)
> -WalkFramesThroughPlaceholders(nsPresContext *aPresContext, nsIFrame *aFrame,
> +WalkFramesThroughPlaceholders(nsPresContext* aPresContext, nsIFrame *aFrame,
>                                frameWalkerFn aFunc, void *aClosure)

I think you can drop the `aClosure` from here and `frameWalkerFn` completely now given nobody uses it anymore.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8950519 [details]
Bug 1437790: No need to reframe synchronously on chrome-flush-skin-caches.

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8950519 [details]
Bug 1437790: No need to reframe synchronously on chrome-flush-skin-caches.

https://reviewboard.mozilla.org/r/219798/#review225818

> I think you can drop the `aClosure` from here and `frameWalkerFn` completely now given nobody uses it anymore.

Yup! done.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36480ec73c27
No need to reframe synchronously on chrome-flush-skin-caches. r=xidorn

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/36480ec73c27