Closed Bug 1241651 Opened 4 years ago Closed 4 years ago

remove nsPresContext::GetDisplayRootPresContext

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- fixed
firefox46 --- fixed
firefox-esr38 45+ fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

Details

Attachments

(3 files)

We have nsPresContext::GetDisplayRootPresContext and nsPresContext::GetRootPresContext. The difference is that GetDisplayRootPresContext looks at the document and asks for it's parent document. This should be unnecessary, and even potentially wrong because if GetParentPresContext() does not find a parent then as far as layout is concerned it does not have a parent document. Because if there is no link via the views that GetParentPresContext looks at then the current prescontext can't be descended into from any parent document via frames or views (ie painting and BuildDisplayList and event targeting).

This was added in http://hg.mozilla.org/mozilla-central/rev/79cd5721b7b4 with the comment "Make GetParentPresContext() succeed when the current PresContext has no frames". The GetParentPresContext of that era

http://hg.mozilla.org/mozilla-central/file/79cd5721b7b4/layout/base/nsPresContext.cpp#l1170

tried to get the root frame of the document, and failed if it couldn't. However, document's don't always have root frames (for example early in initialization). They do always have views. GetParentPresContext was changed to use views (so it didn't fail when there was no root frame) in http://hg.mozilla.org/mozilla-central/rev/3d20e0577cbf. Thus, the originally reason for the creation of GetDisplayRootPresContext is no more.
Attached patch patchSplinter Review
Attachment #8710698 - Flags: review?(matt.woodrow)
Attachment #8710698 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/cc987640612c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment on attachment 8710698 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: existed for a very long time
[User impact if declined]: may fix the crashes of bug 1241217? but we need more data to be sure, uplifted would help with that
[Describe test coverage new/current, TreeHerder]: not specifically tested
[Risks and why]: low risk
[String/UUID change made/needed]: none
Attachment #8710698 - Flags: approval-mozilla-beta?
Comment on attachment 8710698 [details] [diff] [review]
patch

Fix a crash, taking it. Should be in 45 beta 2.
Attachment #8710698 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
[:~/mozilla/unified] $ hg graft -er cc987640612c
grafting 323187:cc987640612c "Bug 1241651. Remove nsPresContext::GetDisplayRootPresContext. r=mattwoodrow"
merging layout/base/nsPresContext.cpp
merging layout/base/nsPresContext.h
merging layout/base/nsRefreshDriver.cpp
merging view/nsView.cpp
warning: conflicts while merging view/nsView.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue

Can we get a rebased patch for beta?
Flags: needinfo?(tnikkel)
Attached patch beta patchSplinter Review
Flags: needinfo?(tnikkel)
Attached patch patch for esr38Splinter Review
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: this is for bug 1241217
User impact if declined: crashes
Fix Landed on Version: on nightly in 46, but it got uplifted
Risk to taking this patch (and alternatives if risky): pretty safe
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8724945 - Flags: approval-mozilla-esr38?
Comment on attachment 8724945 [details] [diff] [review]
patch for esr38

Fixes a sec-crit, meets ESR uplift bar.
Attachment #8724945 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.