227690 - deCOMtaminate callers of nsIPresContext::GetShell()

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Brian Ryner (not reading)]

We should switch all callers of nsIPresContext::GetShell() to the deCOMtaminated
version (GetPresShell).

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Shouldn't that be "PresShell()"?

Comment 3

[Brian Ryner (not reading)]

Could be null (we have a few places that check it and note the circumstances
when it would be null).

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Although GetPresShell() can return null in before attaching the presshell to the
prescontext during initialization, and after PresShell::Destroy(), most call
sites don't check for null.

I guess I want to somehow synchronize the lifetimes of the presshell and the
prescontext so that GetPresShell() never returns null. Not that I have a clear
idea how to do that...

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's a little weird that the presshell holds an owning reference to the
prescontext, but the prescontext exists before and after the preshell. What if
we reversed the ownership relation and created a blank presshell right at the
creation of the prescontext, and held it until the death of the prescontext?

Comment 6

[Brian Ryner (not reading)]

Seems reasonable, but I think the issue is tangential to this patch.  I'd
propose that we get this in and then do the ownership reveral and method
renaming separately (or I could change it to PresShell() in this patch with the
understanding that the behavior will be changed to match later).

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about this plan for this bug: add both GetPresShell() and PresShell(). If a
call site checks for null then it calls GetPresShell() otherwise it calls
PresShell(). PresShell() asserts that the presshell is non-null.

Then we file a new bug calling for synchronization of the prescontext and
presshell lifetimes and the removal of GetPresShell() (and the conversion of
GetPresShell() callers to call PresShell() with no check). This way we don't
have to review another bazillion-line patch.

Also I think it would make sense for me or bz to do r+sr so we don't both have
to crawl through this.

Comment 8

[Brian Ryner (not reading)]

Sounds good to me.  I'll attach a new patch shortly.

Comment 9

[Brian Ryner (not reading)]

Attachment: patch #2

this implements roc's suggestion.  All callers that null-check the result are
using GetPresShell(), everyone else is using PresShell().  I also removed null
presshell assertions at some call sites since we'll now have an assertion in
PresShell().

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 137217 [details] [diff] [review]
patch #2

>Index: content/events/src/nsEventStateManager.cpp
>@@ -4085,8 +4065,7 @@ nsEventStateManager::SendFocusBlur(nsIPr

>+  nsCOMPtr<nsIPresShell> presShell = aPresContext->PresShell();

Add a comment here explaining that we need the ref because we may close the
window partway through this method (I assume that's why we need it, at least).

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>@@ -1547,8 +1547,7 @@ nsGenericHTMLElement::HandleDOMEventForA
>+            nsIPresShell *presShell = aPresContext->GetPresShell();
>             if (presShell) {
>               ret = presShell->HandleDOMEventWithTarget(this, &event, &status);

Add a comment here saying that presShell should NOT be used after the
HandleDOMEventWithTarget call, since it may have been destroyed?  Maybe even
set presShell to null?

>Index: content/xml/content/src/nsXMLElement.cpp
>@@ -446,8 +446,7 @@ nsXMLElement::HandleDOMEvent(nsIPresCont
>+          nsIPresShell *presShell = aPresContext->GetPresShell();
>           if (presShell) {
>             ret = presShell->HandleDOMEventWithTarget(this, &event, 

Same.

>Index: layout/html/base/src/nsHTMLContainerFrame.cpp

>     nsIStyleSet*  styleSet;

Make that an nsCOMPtr while you're here (and remove the manual release)?  Or
even make this use the deCOMified GetStyleSet() version?

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp

>@@ -8008,7 +8004,7 @@ static nsresult InsertOutOfFlow(nsIPresC
>+        AppendFrames(aPresContext, *aPresContext->PresShell(),

*(aPresContext->PresShell()) is probably more clear.

>@@ -8027,7 +8023,7 @@ static nsresult InsertOutOfFlow(nsIPresC
>+    InsertFrames(aPresContext, *aPresContext->PresShell(),

Ditto.

>Index: layout/html/table/src/nsTableFrame.cpp
>@@ -3280,12 +3265,9 @@ nsTableFrame::ReflowChildren(nsIPresCont

>+            aPresContext->PresShell()->GetStyleSet(&styleSet);

Same as above comment about nsIStyleSet (in nsHTMLContainerFrame.cpp).

For that matter, can a presshell exist without a style set?  If not, we should
munge that accessor too (in a separate bug).

>Index: layout/xul/base/src/nsBoxLayoutState.cpp

> nsBoxLayoutState::GetPresShell(nsIPresShell** aShell)
>+  if (mPresContext) {

None of the other methods you changed null-check mPresContext.	Can it ever be
null?

Does this class assert when inited with a null prescontext?  If not, it
probably should....

>Index: layout/xul/base/src/nsButtonBoxFrame.cpp

>@@ -182,9 +180,8 @@ nsButtonBoxFrame::MouseClicked (nsIPresC
>     shell->HandleDOMEventWithTarget(mContent, &event, &status);

Again, comment that "shell" may be garbage after that call.  Or set it to null.

>Index: layout/xul/base/src/nsMenuFrame.cpp
>@@ -1661,10 +1657,9 @@ nsMenuFrame::Execute(nsGUIEvent *aEvent)
>+  nsCOMPtr<nsIPresShell> shell = mPresContext->GetPresShell();

Comment that this needs to be a strong ref because it's needed after the event
dispatch (I assume that's why?).

>@@ -1702,10 +1697,10 @@ nsMenuFrame::OnCreate()
>       rv = shell->HandleDOMEventWithTarget(child, &event, &status);

Comment that "shell" is garbage after this or set to null.

>@@ -1798,10 +1793,9 @@ nsMenuFrame::OnCreated()

Ditto.

>@@ -1832,10 +1826,9 @@ nsMenuFrame::OnDestroy()

Ditto.

>@@ -1866,10 +1859,9 @@ nsMenuFrame::OnDestroyed()

Ditto.

>@@ -2063,10 +2055,8 @@ nsMenuFrame::SetActiveChild(nsIDOMElemen
>+  mPresContext->GetPresShell()->GetPrimaryFrameFor(child, &kid);

That should use PresShell(), not GetPresShell().


>Index: layout/xul/base/src/nsPopupSetFrame.cpp
>@@ -590,15 +586,14 @@ nsPopupSetFrame::OnCreate(PRInt32 aX, PR
>+      nsresult rv = shell->HandleDOMEventWithTarget(aPopupContent, &event,

Comment that "shell" is garbage after this or set to null.

>@@ -675,14 +670,13 @@ nsPopupSetFrame::OnCreated(PRInt32 aX, P
>+      nsresult rv = shell->HandleDOMEventWithTarget(aPopupContent, &event,

Ditto.

>@@ -703,13 +697,13 @@ nsPopupSetFrame::OnDestroy(nsIContent* a
>+      nsresult rv = shell->HandleDOMEventWithTarget(aPopupContent, &event,

Ditto.

>@@ -729,13 +723,13 @@ nsPopupSetFrame::OnDestroyed(nsIContent*
>+      nsresult rv = shell->HandleDOMEventWithTarget(aPopupContent, &event,

Ditto.

With those changes, r+sr=bzbarsky.  Nice cleanup!

Comment 11

[Brian Ryner (not reading)]

Checked in, and filed bug 229080 on keeping the object lifetimes in sync.

Comment 12

[Frederic Bezies]

Kills 0.8 firebird branch building process -> bug 229090

Comment 13

[noririty]

This patch caused Bug 229624.