Closed Bug 1350671 Opened 8 years ago Closed 8 years ago

stylo: PresShell::RecreateFramesFor is sometimes called when there are pending restyles

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(1 file)

Bug 1350671 - stylo: Allow resolving out of date styles when explicitly reconstructing frames for an element.
59 bytes, text/x-review-board-request
bholley
: review+
Details
layout/reftests/bugs/289480.html triggers the assertions in bug 1345695 about non-lazy restyles. The test has an <object> element, and when the referenced file starts to load, we land in nsObjectLoadingContent::NotifyStateChanged. This calls UpdateState() on the element, which causes an ElementSnapshot to be created. Then we call PresShell::RecreateFramesFor. During frame reconstruction, we grab styles off the element without allowing lazy restyling. Due to the presence of the ElementSnapshot, we think the styles are out of date, so we assert. I don't think it matters much that we get out of date styles during the explicit frame reconstruction, but it's probably not worth the effort of passing through state to allow it. Instead, we can just flush styles inside PresShell::RecreateFramesFor.
Hmm, that seems to have caused some assertions in a few mochitests in non-stylo builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c5ce3fe6918ffc557764dd43125ba744b3fb957&group_state=expanded&selectedJob=86493035 ###!!! ASSERTION: Unexpected document; this will lead to incorrect behavior!: 'aElement->GetComposedDoc() == Document()', file /z/mozilla-central/layout/base/RestyleTracker.cpp, line 39 ###!!! ASSERTION: Have parent context and shouldn't: 'Error', file /z/mozilla-central/layout/base/RestyleManager.cpp, line 607 ###!!! ASSERTION: Wrong parent style context: 'Error', file /z/mozilla-central/layout/base/RestyleManager.cpp, line 595 ###!!! ASSERTION: frame still referencing style context: 'mFrameRefCnt == 0', file /z/mozilla-central/layout/style/nsStyleContext.cpp, line 1588 Something to do with the XML pretty printing binding. So maybe we should preserve the existing behaviour and allow existing (but out of dates) styles under PresShell::RecreateFramesFor.
Attachment #8851339 - Flags: review?(bobbyholley)
Summary: stylo: flush styles before explicitly recreating frames → stylo: PresShell::RecreateFramesFor is sometimes called when there are pending restyles
Comment on attachment 8851339 [details] Bug 1350671 - stylo: Allow resolving out of date styles when explicitly reconstructing frames for an element. https://reviewboard.mozilla.org/r/123658/#review126578 ::: layout/style/ServoStyleSet.h:323 (Diff revision 2) > nsPresContext* mPresContext; > UniquePtr<RawServoStyleSet> mRawSet; > EnumeratedArray<SheetType, SheetType::Count, > nsTArray<RefPtr<ServoStyleSheet>>> mSheets; > int32_t mBatching; > + bool mAllowResolveExistingStyles; I think we should rename "Existing" to "Stale" here and everywhere else in this patch. ::: servo/ports/geckolib/glue.rs:1475 (Diff revision 2) > + if !data.has_styles() { > + warn!("Resolving existing style on unstyled element with lazy computation forbidden."); > let per_doc_data = PerDocumentStyleData::from_ffi(raw_data).borrow(); > return per_doc_data.default_computed_values().clone().into_strong(); > } Do we also hit this, or can we assert against this case? If we do hit it, we should fix up these cases as well as part of the work you're doing. I guess this should probably be called Servo_ResolvePossiblyStaleStyle. I think I'd actually prefer to pass a boolean of aAllowStale to Servo_ResolveStyle than to introduce this additional API. I'll leave it up to you though.
Attachment #8851339 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7) > Do we also hit this, or can we assert against this case? If we do hit it, we > should fix up these cases as well as part of the work you're doing. In my testing patches I have this converted to an assertion, so I will find out.
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6e4ccddb5c4b stylo: Allow resolving out of date styles when explicitly reconstructing frames for an element. r=bholley
Assignee: nobody → cam
Priority: -- → P1
Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e05359a30577 stylo: Allow resolving out of date styles when explicitly reconstructing frames for an element. r=bholley
https://hg.mozilla.org/mozilla-central/rev/e05359a30577
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(cam)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: