488771 - frame classes should not implement nsISupports

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Frame classes should not implement nsISupports.  If they're passed to things that expect reference-counting semantics, we could end up with dangling pointers.

I see the following frame classes that still implement AddRef, Release, or QueryInterface:

nsIsIndexFrame, because it implements nsIDOMKeyListener
nsGfxScrollFrameInner, because it implements nsIScrollPositionListener
CanvasFrame (in nsHTMLFrame.cpp), because it implements nsIScrollPositionListener
nsImageFrame, for no good reason anymore
nsLeafBoxFrame, for no good reason anymore

We should move the implementations of these interfaces onto other objects (or potentially remove reference-counting semantics and nsISupports inheritance from nsIScrollPositionListener), and then remove the QueryInterface, AddRef, and Release implementations from all of these objects.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oops, missed one:

nsListControlFrame, for no good reason anymore

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I put a patch to deal with the unused ones in bug 488774.

Comment 3

[Benjamin Smedberg]

The following classes derive from nsISupports and nsIFrame, according to the static analysis warnings in the tree:

nsComboboxControlFrame
nsIsIndexFrame
CanvasFrame
nsMathMLmactionFrame

We have warnings for these: http://office.smedbergs.us:8080/search?user=&path=&msg=nsIFrame%25&id=1312

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yeah, I forgot to search for the macros that declared the nsISupports methods...

nsComboboxControlFrame is because of nsIRollupListener and nsIScrollPositionListener

nsMathMLmactionFrame is because of nsIDOMMouseListener

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #4)
> nsComboboxControlFrame is because of nsIRollupListener and
> nsIScrollPositionListener

Er, sorry, only nsIRollupListener.  (I saw nsIScrollableViewProvider and though it was nsIScrollPositionListener.)

Comment 6

[Benjamin Smedberg]

I can turn the warnings into errors when this is done.

Comment 7

[Jesse Ruderman]

Did zwol's queryframe work in bug 497495 help here?

Or is this a matter of removing inheritance arrows?  Is there a bug on each one?

Comment 8

[Zack Weinberg (:zwol)]

My queryframe work doesn't help here, sorry.

Comment 9

[Jesse Ruderman]

The static analysis only complains about CanvasFrame, nsComboboxControlFrame, and nsIsIndexFrame.  Is nsMathMLmactionFrame in the clear?

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1256968831.1257009300.3018.gz&fulltext=1

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #9)
> The static analysis only complains about CanvasFrame, nsComboboxControlFrame,
> and nsIsIndexFrame.  Is nsMathMLmactionFrame in the clear?

No.  The code is still clearly bad.

Comment 11

[Timothy Nikkel (:tnikkel)]

I imagine the best way to go about this would be to split off a bug for each class? Do those bugs need to be security sensitive too?

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: nsMathMLmactionFrame

Comment 13

[Timothy Nikkel (:tnikkel)]

Attachment: nsIsIndexFrame

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: nsComboboxControlFrame

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: nsIScrollPositionListener

Comment 16

[Timothy Nikkel (:tnikkel)]

I happened to see your nsIScrollPositionListener changes in the try server queue, so one of us will have to merge.

Comment 17

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

+void
+ShowStatus(nsPresContext* aPresContext, nsString& aStatusMsg)

Make it 'static'

+  if (mListener)
+    mContent->RemoveEventListenerByIID(mListener, NS_GET_IID(nsIDOMMouseListener));

Clear out mListener->mOuter, and null-check mOuter where we use it, so that even if someone gets a reference to the mListener they can't trick us into crashing.

+    NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent);// { MOUSE(click) return NS_OK; }

Why are you commenting this out? I'd just remove these.

In fact, I'd remove all the DEBUG_mouse stuff.

Comment 18

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

(In reply to comment #17)
> Clear out mListener->mOuter, and null-check mOuter where we use it, so that
> even if someone gets a reference to the mListener they can't trick us into
> crashing.

Similar deal for the KeyListener in nsIsIndexFrame.

Can we make nsIRollupListener not inherit from nsISupports?

The nsIScrollPositionListener is going to cause a ton of conflicts with patches in my tree, I'd rather deal with that a little later.

Comment 19

[Timothy Nikkel (:tnikkel)]

(In reply to comment #17)
> +void
> +ShowStatus(nsPresContext* aPresContext, nsString& aStatusMsg)
> 
> Make it 'static'

Ok.

> +  if (mListener)
> +    mContent->RemoveEventListenerByIID(mListener,
> NS_GET_IID(nsIDOMMouseListener));
> 
> Clear out mListener->mOuter, and null-check mOuter where we use it, so that
> even if someone gets a reference to the mListener they can't trick us into
> crashing.

Done.
But you mean mOwner? Or do you want me to change the name too?

> +    NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent);// { MOUSE(click) return
> NS_OK; }
> 
> Why are you commenting this out? I'd just remove these.
> 
> In fact, I'd remove all the DEBUG_mouse stuff.

Ok.

(In reply to comment #18)
> (In reply to comment #17)
> > Clear out mListener->mOuter, and null-check mOuter where we use it, so that
> > even if someone gets a reference to the mListener they can't trick us into
> > crashing.
> 
> Similar deal for the KeyListener in nsIsIndexFrame.

Ok.

> Can we make nsIRollupListener not inherit from nsISupports?

I looked into this, but there would be code in a bunch of places to change on each platform. Can we just go with this more limited change?

> The nsIScrollPositionListener is going to cause a ton of conflicts with patches
> in my tree, I'd rather deal with that a little later.

Ok. Keep me in on the loop when the conflicting stuff gets landed.

Comment 20

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

(In reply to comment #19)
> But you mean mOwner? Or do you want me to change the name too?

No, just my mistake.

> > Can we make nsIRollupListener not inherit from nsISupports?
> 
> I looked into this, but there would be code in a bunch of places to change on
> each platform. Can we just go with this more limited change?

We could, but then we'd just have to undo it again so we don't create a separate object etc. I'd rather just do it right. That doesn't mean you have to do it.

> > The nsIScrollPositionListener is going to cause a ton of conflicts with
> > patches in my tree, I'd rather deal with that a little later.
> 
> Ok. Keep me in on the loop when the conflicting stuff gets landed.

Sure will!

Thanks!

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: nsMathMLmactionFrame v2

Comment 22

[Timothy Nikkel (:tnikkel)]

Attachment: nsIsIndexFrame v2

Comment 23

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

Comment on attachment 409656 [details] [diff] [review]
nsMathMLmactionFrame v2

+    if (mListener)
+      mContent->AddEventListenerByIID(mListener, NS_GET_IID(nsIDOMMouseListener));

{}

+  if (mOwner)
+    mOwner->MouseOver();

{}

+  if (mOwner)
+    mOwner->MouseOut();

{}

+  if (mOwner)
+    mOwner->MouseClick();

{}

Comment 24

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

Comment on attachment 409657 [details] [diff] [review]
nsIsIndexFrame v2

+  if (mListener)
+    mInputContent->AddEventListenerByIID(mListener, NS_GET_IID(nsIDOMKeyListener));

{}

+  if (mOwner)
+    mOwner->KeyPress(aEvent);

{}

Comment 25

[Timothy Nikkel (:tnikkel)]

Attachment: nsIRollUpListener

I figured it wouldn't be as bad if I didn't change the method signatures by removing the COM from them. But I ran into the issue of needing to QI to nsIMenuRollup from the nsIRollupListener so I had to do a bunch more.

This fails to build on OS X, it's probably something trivial, but I give up trying to debug this via the tryserver (at least for now). Log showing the error:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1257224062.1257225839.30302.gz

Comment 26

[Timothy Nikkel (:tnikkel)]

Well, I don't know for sure that the pasted link to the log represents the current error, as my last push-to-try failed to build on OS X but links to the logs are no where to be found on the tinderbox page.

Comment 27

[Timothy Nikkel (:tnikkel)]

When landing these, can I use a normal commit message? ie "Bug 488771. Stop ... from inheriting from nsISupports. r=roc" Or do I need any misdirection from this being a security bug?

Comment 28

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

I wouldn't bother with misdirection here.

Comment 29

[Timothy Nikkel (:tnikkel)]

Attachment: nsIRollUpListener v2

This passes try server.

Since I didn't get any of the 3 main platforms building on the first try it is highly likely that this will break the other platforms. Which platforms have (semi-)active maintainers that we should notify?

Comment 30

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

OS/2 and Qt. I don't remember email addresses off the top of my head, but a little Mercurial archaeology should suffice.

Comment 31

[Timothy Nikkel (:tnikkel)]

nsIsIndexFrame
http://hg.mozilla.org/mozilla-central/rev/813133c740d8

nsMathMLmactionFrame
http://hg.mozilla.org/mozilla-central/rev/54c123e95d10

Comment 32

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

Looks good, but I believe global nsCOMPtrs (and other C++ types with nontrivial constructors/destructors) are forbidden. So you should use manual refcounting for gMenuRollup/sMenuRollup/etc. (I know we're already using a global nsCOMPtr in a few places, sorry).

Comment 33

[Timothy Nikkel (:tnikkel)]

Attachment: nsIRollUpListener v3

Got rid of the global nsCOMPtrs.

Comment 34

[Timothy Nikkel (:tnikkel)]

Peter and Tobias, the nsIRollUpListener v3 patch makes changes to all platforms, I would appreciate it if you could take a look so I don't break your platform.

Comment 35

[Peter Weilbacher]

Thanks for asking! :-)
OS/2 builds at least still build widget, that's at least a good sign.

Comment 36

[Timothy Nikkel (:tnikkel)]

nsIRollUpListener
http://hg.mozilla.org/mozilla-central/rev/c7312c8d5c28

Comment 37

[Michael Wu [:mwu]]

Comment on attachment 410554 [details] [diff] [review]
nsIRollUpListener v3

>diff --git a/widget/src/qt/nsWindow.cpp b/widget/src/qt/nsWindow.cpp
>--- a/widget/src/qt/nsWindow.cpp
>+++ b/widget/src/qt/nsWindow.cpp
>@@ -865,16 +870,17 @@ check_for_rollup(double aMouseX, double 
>             if (rollup) {
>                 gRollupListener->Rollup(popupsToRollup, nsnull);
>                 retVal = PR_TRUE;
>             }
>         }
>     } else {
>         gRollupWindow = nsnull;
>         gRollupListener = nsnull;
>+        gMenuRollup = nsnull;

Should this be NS_IF_RELEASE(gMenuRollup);? check_for_rollup in gtk does it.

Comment 38

[Timothy Nikkel (:tnikkel)]

(In reply to comment #37)
> Should this be NS_IF_RELEASE(gMenuRollup);? check_for_rollup in gtk does it.

Yes, thanks for catching this.

Comment 39

[Timothy Nikkel (:tnikkel)]

Attachment: nsIRollUpListener followup

Fix issue in comment 37, and fix up a few tabs I screwed up.

Comment 40

[Timothy Nikkel (:tnikkel)]

Attachment: nsIScrollPositionListener v2

Comment 41

[Timothy Nikkel (:tnikkel)]

nsIScrollPositionListener
http://hg.mozilla.org/mozilla-central/rev/3f8d4e7bd9a1

nsIRollUpListener followup
http://hg.mozilla.org/mozilla-central/rev/dece4c52cbec

And this bug should be done now.