Closed Bug 488771 Opened 11 years ago Closed 10 years ago

frame classes should not implement nsISupports

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: dbaron, Assigned: tnikkel)

Details

(Whiteboard: [sg:critical?])

Attachments

(5 files, 6 obsolete files)

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.
Oops, missed one:

nsListControlFrame, for no good reason anymore
Flags: blocking1.9.2?
I put a patch to deal with the unused ones in bug 488774.
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
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
(In reply to comment #4)
> nsComboboxControlFrame is because of nsIRollupListener and
> nsIScrollPositionListener

Er, sorry, only nsIRollupListener.  (I saw nsIScrollableViewProvider and though it was nsIScrollPositionListener.)
I can turn the warnings into errors when this is done.
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-
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?
Whiteboard: [sg:critical?]
My queryframe work doesn't help here, sorry.
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
(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.
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?
Attached patch nsMathMLmactionFrame (obsolete) — Splinter Review
Assignee: nobody → tnikkel
Attachment #409646 - Flags: review?(roc)
Attached patch nsIsIndexFrame (obsolete) — Splinter Review
Attachment #409647 - Flags: review?(roc)
Attached patch nsComboboxControlFrame (obsolete) — Splinter Review
Attachment #409648 - Flags: review?(roc)
Attached patch nsIScrollPositionListener (obsolete) — Splinter Review
Attachment #409649 - Flags: review?(roc)
I happened to see your nsIScrollPositionListener changes in the try server queue, so one of us will have to merge.
+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.
(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.
(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.
(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!
Attachment #409646 - Attachment is obsolete: true
Attachment #409656 - Flags: review?(roc)
Attachment #409646 - Flags: review?(roc)
Attachment #409647 - Attachment is obsolete: true
Attachment #409657 - Flags: review?(roc)
Attachment #409647 - Flags: review?(roc)
Attachment #409648 - Flags: review?(roc)
Attachment #409649 - Flags: review?(roc)
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();

{}
Attachment #409656 - Flags: review?(roc) → review+
Comment on attachment 409657 [details] [diff] [review]
nsIsIndexFrame v2

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

{}

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

{}
Attachment #409657 - Flags: review?(roc) → review+
Attached patch nsIRollUpListener (obsolete) — Splinter Review
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
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.
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?
I wouldn't bother with misdirection here.
Attached patch nsIRollUpListener v2 (obsolete) — Splinter Review
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?
Attachment #409648 - Attachment is obsolete: true
Attachment #409891 - Attachment is obsolete: true
Attachment #410170 - Flags: review?(roc)
OS/2 and Qt. I don't remember email addresses off the top of my head, but a little Mercurial archaeology should suffice.
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).
Got rid of the global nsCOMPtrs.
Attachment #410170 - Attachment is obsolete: true
Attachment #410554 - Flags: review?(roc)
Attachment #410170 - Flags: review?(roc)
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.
Thanks for asking! :-)
OS/2 builds at least still build widget, that's at least a good sign.
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.
(In reply to comment #37)
> Should this be NS_IF_RELEASE(gMenuRollup);? check_for_rollup in gtk does it.

Yes, thanks for catching this.
Fix issue in comment 37, and fix up a few tabs I screwed up.
Attachment #420828 - Flags: review?(roc)
Attachment #409649 - Attachment is obsolete: true
Attachment #424142 - Flags: review?(roc)
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Group: core-security
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.