396185 - Make nsIFrame not inherit from nsISupports

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Benjamin Smedberg]

Attachment: Make nsIFrame not inherit from nsISupports, rev. 1

This is a prerequisite for the XPCOMGC work: currently nsIFrame and therefore all frames inherit from nsISupports. In the new brave world of MMgc, this is really bad, because we will start trying to hold writebarriers to these objects from some places.

The attached patch makes nsIFrame inherit from an arbitrary new interface nsFrameQI which supports a variety of QI but doesn't have addref/release or the XPCOM-but-not-really baggage that goes with it.

Note that the do_QueryInterface call that take nsFrameQI is not null-safe. When converting old code I errred on the side of caution when I wasn't sure whether a source pointer could be null or not.

There are still a few cases of frames that implement real XPCOM interfaces, including:

nsComboButtonListener: nsIDOMMouseListener
nsSVGPaintServerFrame: nsISVGValue
nsComboboxControlFrame: nsIRollupListener
nsIsIndexFrame: nsIDOMKeyListener
nsMathMLmactionFrame: nsIDOMMouseListener
nsSVG*Frame: nsISVGValueObserver
nsTableRowGroupFrame: nsILineIteratorNavigator
nsAutoRepeatBoxFrame: nsITimerCallback

These are going to be unsafe from GC even if they aren't unsafe from XPCOM now, and will file followup to make these some kind of frame proxy object.

I am definitely requesting review for mozilla-central and I'd also like an evaluation of whether this would be good to land for 1.9: I think the changed QueryInterface signature without the outparam could be good for perf, but I haven't measured.

Comment 1

[Benjamin Smedberg]

Attachment: Make nsIFrame not inherit from nsISupports, rev. 1.1

Whoops, forgot to hg qrefresh before uploading the patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

So..  I don't like this sort of thing:

+    nsIScrollableViewProvider* svp =
+      do_QueryInterface<nsIScrollableViewProvider>(scrollFrame);

It has the undesirable property of having two places which specify nsIScrollableViewProvider and the two could get out of sync.  We've had exactly this problem with explicit QueryInterface calls.  What can we do to get back the automatic type-safety we have on trunk now with do_QI and CallQI?  Since we need extra text anyway, I would rather have a decoration on the thing I'm assigning to if that's what's needed to make this work.

Why did you make nsIPhonetic no longer scriptable?  If we want to eliminate it, we should just do it instead of leaving it around but not usable for its original purpose.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Frankly, I think just using CallQI throughout with it being (as now) a templated thing that inlines to whatever the typesafe thing is might be the simplest thing to do...

Comment 4

[Benjamin Smedberg]

Please note that this version of do_QueryInterface is typesafe. You cannot do, for example:

nsInlineFrame* inline = do_QueryInterface<nsBlockFrame>(frame);

It's unfortunate that we have to specify the template type, but according to the C++ spec you're not allowed to deduce a function template type from only the return type.

The existing CallQueryInterface pattern is very non-optimal because it uses an out-parameter, which I've otherwise eliminated from the QI codepath.

I made nsIPhonetic no longer scriptable because it doesn't inherit from nsISupports. I didn't remove it entirely because it's used in editor and I didn't want to get into a bigger refactoring game than necessary.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Please note that this version of do_QueryInterface is typesafe.

Ah, ok.  That's good to know.

> it uses an out-parameter

But the inlining optimizes it away, right?  And it's shorter to type....

You shouldn't have changed the inheritance of nsIPhonetic, imo.  You _should_ have introduced shims that implement it in the frame code. We have a bug on this.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Another thought... Would it be possible to have something more like:

  do_QueryInterface<nsIScrollableViewProvider> svp(scrollFrame);

The assignment operator could do the QI automatically.

Comment 7

[Benjamin Smedberg]

Note, this patch was developed on Mac so there's some win/gtk specific (native theming) code that I'll need to fix up as well.

Comment 8

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

Olli might be interested in helping get rid of the XPCOM interfaces on frames --- they're definitely bad news.

I don't know what nsIPhonetic is used for --- anyone? For the frame side at least, it seems GetPhonetic could be moved to nsIFormControlFrame so frames don't have to implement nsIPhonetic.

nsILineIterator is a crappy interface and should probably just become non-XPCOM. It is only implemented by frames.

The others should be fixed by introducing a proxy object, like we've done in many places to fix crashes/security bugs.

So this usage of do_QueryInterface builds on trunk?

Comment 9

[Benjamin Smedberg]

Yes, this patch builds and works on trunk mac. Two reftests had off-by-a-pixel errors, but that may just be fonts on my local machine.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(see also bug 385843 about nsIPhonetic)

Comment 11

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

How about bz's suggestion from comment #6? I'd call it something other than do_QueryInterface though.

Should we generalize nsFrameQI a bit in case something else wants to use it? I suspect we may find other needs for it. For example views could use it (although they're going away post-1.9...)

Comment 12

[Benjamin Smedberg]

Yes, bz's suggestion will work for at least the initializer cases. Do you want me to do that instead? I kinda wish we didn't because the pointer is just a pointer in debuggers, rather than having to deal with mRealPtr and template functions, which most debuggers get at least kinda wrong.

Comment 13

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

Call it mRawPtr at least. I'm used to it... I think it's worth not typing the type twice.

Comment 14

[Benjamin Smedberg]

Attachment: Updated, still mac-only

ok, this has the wrapper-class changes... it doesn't change the other bits like nsIPhonetic, because I'd prefer not to make additional changes in this patch unless absolutely necessary.

Comment 15

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

What do you think about generalizing (the names of) nsFrameQI and nsFramePtr so that we can use them in other contexts?

Comment 16

[Benjamin Smedberg]

In terms of automatic rewriting I'd prefer other contexts to be separate: that way we know what ownership model is in force. What are the other proposed contexts anyway?

Comment 17

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

Views is one, although we'll remove those for Mozilla2 anyway. I suspect as we GC more and refcount less, we'll find more situations where we still want to QI.
But this is OK. I guess later if we want to we could make nsFrameQI and nsFramePtr inherit from some common base nsQI or something.

After I review this, are you going to do another round with the non-Mac stuff and ask for review on that separately? or what?

Comment 18

[Benjamin Smedberg]

Yes I will, I have this as an mqueue so I can put up the remaining bits separately... turns out there's some accessibility wonkiness too.

Comment 19

[Boris Zbarsky [:bzbarsky]]

This is still changing things so that one can no longer use nsIPhonetic on an editor from script anymore.  At which point, it'll basically be unusable and might as well be removed entirely, no?

Comment 20

[Benjamin Smedberg]

Comment on attachment 281746 [details] [diff] [review]
Updated, still mac-only

jorendorff gave me a tip on how to make the do_QueryInterface magic work. I'm going to do that and submit a complete rollup patch.

Comment 21

[Jason Orendorff [:jorendorff]]

(In reply to comment #20)

Well, admittedly I couldn't keep my mouth shut since there was a C++ puzzle on
the table... but in the long run, I would rather avoid depending on this kind
of magic.  I think we stand to lose something from having so many "cute tricks"
in our code.  But I guess that's really a separate discussion.

Comment 22

[Benjamin Smedberg]

Attachment: Complete, all platforms, rev. 1

This is the completed patch with the jorendorff magic, ready for review (for mozilla-central)

Comment 23

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

Overloading the same function name (do_QueryInterface) that we use for nsCOMPtr seems like a bad idea.

I'm also concerned about the size of this patch if it lands on Mozilla 2 and not 1.9.  It could have been done significantly more conservatively -- e.g., without changing the signature of QueryInterface.  I think if this patch lands on one and not the other, it will require significant layout expertise to do some of the merging necessary.  Since I don't think anybody with that layout expertise is currently working on Mozilla 2, or will be for quite a while, I think it significantly increases the chances that the fork will be permanent.

Comment 24

[Benjamin Smedberg]

It is not possible to do this patch correctly while keeping the same signature for QueryInterface: I have to keep the COM QI impls entirely separate from the frame QI impls, in order for the next step of automatic rewriting to work correctly.

Actually I should probably change the name of nsFrameQI::QueryInterface altogether because the COM QI signature may change and then we'd have matching signature again.

It is theoretically possible to write an overloaded CallQueryInterface signature:

template<class D>
void
CallQueryInterface(nsFrameQI* source, D* *destination)
{
  *destination =
    static_cast<D*>(source->QueryInterface(NS_GET_TEMPLATE_IID(D)));
}

And when this is inlined it could end up the same as the direct version.

Frankly unless layout is planning significant frame-hierarchy changes for 1.9(.1) I don't think the merge conflicts from this would be difficult at all; the callsite changes are very straightforward: the only place where things really get tricky is the implementations of COMQI/FrameQI.

Comment 25

[Brendan Eich [:brendan]]

There's not going to be a layout fork, and we are deCOMtaminating soon. Talking about a fork is not productive. The fact is we'll need layout experise on Mozilla 2 soon. If anyone thinks we should make major pre-Mozilla-2 changes to layout, please mail me.

/be

Comment 26

[Benjamin Smedberg]

roc/dbaron, I'd like some guidance on how to proceed. I can rename QueryInterface->QueryFrame and do_QueryInterface->do_QueryFrame fairly easily, but I don't want to make any further changes to this (not autogenerated) patch without guidance as to what will be acceptable.

Comment 27

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

New names are probably good for everyone, but as I mentioned, I would like names that aren't frame-specific in case we switch other classes from refcounting to GC that need some kind of queryinterface capability.

I'm not worried about the difficulty of merges, just their quantity. How about this: Land a minimal patch with compatible CallQueryInterface for mozilla2 now. Defer further changes (new names and elimination of CallQueryInterface) until as late as possible. Does that sound reasonable?

Comment 28

[Brendan Eich [:brendan]]

We need at least private-repo-hosted deCOMtamination patches that are aggressive and may cause merge hardships for some folks, but these need not be permanent if taras's tools can be rerun to generate new patches. Right now, we really want to get working builds, codesighs results, and perf/footprint results. So no one should panic about a big merge tax.

We will diverge at some point, but two thoughts:

1. Rerun taras's tools when necessary against trunk to make a less conflicty patch, apply that (i.e., use patch, not hg merging).

2. Share the merge work. It does not all require layout gurus, all the time, for all hunks in conflict.

We really want to get upper-bound codesighs wins, so some amount of hacking should be done very soon, even if it results in leaky or buggy code. E.g., systematic sresult removal (out param becomes return value) plus exception handling, measure codesigh, note outstanding bugs, remove patch and iterate on tooling (for RAII and other exception safety).

I'll be back at work this week and poking around to help get such numbers.

All this means that no one should panic about merge conflicts. But, we do need a coordinated roadmap for how to develop post-1.9 without forking into two major lines of development, while making good use of hg.mozilla.org and trading off merge conflicts and progress on Mozilla 2 well. I'll be working on that too.

On a more generic name than QueryFrame, some candidates:

Query (too short and overloaded!)
QueryBase
QueryAbstractBase
QueryAbstaction
QueryForIID (yechh!)
DynamicCast

I'm currently in favor of the last one, simply by hating the others more.

/be

Comment 29

[Benjamin Smedberg]

Please note that this patch is not auto-generated

As I've continued to develop stuff like this, I think the base class should be frame-specific. It is possible/likely that we can use integers to identify frames in the future, instead of expensive IIDs. I think we should use separate base classes for each ownership model. Thus my increasingly strong preference for QueryFrame

Comment 30

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

Okay then, I accept that.

Comment 31

[Brendan Eich [:brendan]]

(In reply to comment #30)
> Okay then, I accept that.

Whew! Spares us from DynamicCast. :-)

/be

Comment 32

[Uri Bernstein]

Comment on attachment 282613 [details] [diff] [review]
Complete, all platforms, rev. 1

>diff -r 45182e442e21 -r b03452777fd4 layout/generic/nsFrame.cpp
>--- a/layout/generic/nsFrame.cpp	Tue Sep 25 20:49:12 2007 -0400
>+++ b/layout/generic/nsFrame.cpp	Thu Sep 27 18:32:00 2007 -0400
>@@ -5078,15 +5043,11 @@ nsIFrame::GetFrameFromDirection(nsDirect
...
>-    nsISupports *isupports = nsnull;
>-    result = frameTraversal->CurrentItem(&isupports);
>-    if (NS_FAILED(result))
>-      return result;
>-    if (!isupports)
>+    traversedFrame = frameTraversal->CurrentItem();
>+    if (!traversedFrame)
>       return NS_ERROR_NULL_POINTER;
>     //we must CAST here to an nsIFrame. nsIFrame doesn't really follow the rules
>     //for speed reasons
>-    traversedFrame = (nsIFrame *)isupports;

You should probably remove the "//we must CAST..." comment here (I never quite understood it).

>diff -r 45182e442e21 -r b03452777fd4 layout/generic/nsSelection.cpp
>--- a/layout/generic/nsSelection.cpp	Tue Sep 25 20:49:12 2007 -0400
>+++ b/layout/generic/nsSelection.cpp	Thu Sep 27 18:32:00 2007 -0400
>@@ -2061,14 +2060,11 @@ nsFrameSelection::GetFrameFromLevel(nsIF
...
>-    result = frameTraversal->CurrentItem(&isupports);
>-    if (NS_FAILED(result))
>-      return result;
>-    if (!isupports)
>+    foundFrame = frameTraversal->CurrentItem();
>+    if (!foundFrame)
>       return NS_ERROR_NULL_POINTER;
>     //we must CAST here to an nsIFrame. nsIFrame doesn't really follow the rules
>     //for speed reasons
>-    foundFrame = (nsIFrame *)isupports;

Same here.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree with bsmedberg. Ideally we would have an enum with the various frame types which could include both frame "interfaces" as well as concrete frame classes. That should make things clearer and faster.

Comment 34

[Benjamin Smedberg]

Attachment: Checkpoint, builds and runs on Linux

This is the new work in progress... it's a lot cleaner than the old one, and the dependent work has been moved into other bugs which can land first. It appears to basically run on Linux, though I haven't done thorough unit-test runs on it yet, and it's likely to break on mac/windows in platform-specific accessibility and menu code.

Comment 35

[[e-mail]]

>+class nsQueryFrameResult
>+{
>+public:
>+  nsQueryFrameResult(nsQueryFrame *s) : mRawPtr(s) { }
>+
>+  template<class Dest>
>+  operator Dest*() {
>+    if (!mRawPtr)
>+      return nsnull;
>+
>+    return reinterpret_cast<Dest*>(mRawPtr->QueryFrame(Dest::kFrameIID));
>+  }
>+
>+private:
>+  nsQueryFrame *mRawPtr;
>+};
>+
>+inline nsQueryFrameResult do_QueryFrame(nsQueryFrame *f)
>+{
>+  return nsQueryFrameResult(f);
>+}
Might as well cut out the middle man and name the class do_QueryFrame ;-)

Comment 36

[Benjamin Smedberg]

Attachment: nsQueryFrame for review, rev. 1

This patch builds on all three platforms (at least in local testing). I'm having trouble getting meaningful reftests currently (a totally clean build fails hundreds of them on Linux, so I'm going to try mac a bit later)... but this should be ready for review.

Comment 37

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

Comment on attachment 346532 [details] [diff] [review]
nsQueryFrame for review, rev. 1

-  return layoutObject->QueryInterface(NS_GET_IID(nsITableLayout), 
-                                      (void**)(tableLayoutObject)); 
+  *tableLayoutObject = do_QueryFrame(layoutObject);
+  return NS_OK;

This is possibly a behaviour change --- slightly safer to "return *tableLayoutObject ? NS_OK : NS_NOINTERFACE;"

-      nsresult rv = NS_ERROR_FAILURE;
       while (frame &&
-             frame->IsFrameOfType(nsIFrame::eLineParticipant) &&
-             NS_FAILED(rv)) {
+             frame->IsFrameOfType(nsIFrame::eLineParticipant) && !mBlockFrame) {
         frame = frame->GetParent();
-        rv = frame->QueryInterface(kBlockFrameCID, (void**)&mBlockFrame);
+        mBlockFrame = do_QueryFrame(frame);
       }
-      NS_ASSERTION(NS_SUCCEEDED(rv) && mBlockFrame, "Cannot find containing block.");
+      NS_ASSERTION(mBlockFrame, "Cannot find containing block.");

You need to set mBlockFrame to nsnull before entering the loop. It should probably be a do-while...

+#ifndef nsQueryFrame_h__
+#define nsQueryFrame_h__

C++ fascists tell me that __ is prohibited anywhere in the identifier. I guess nsQueryFrameh_h_ works

+class nsQueryFrame
+{
+public:
+  enum FrameIID {
+    BRFrame_id,
+    CanvasFrame_id,
+    nsAreaFrame_id,
+    nsAutoRepeatBoxFrame_id,

I love the smell of brute force in the morning!

Otherwise fabulous.

Comment 38

[Benjamin Smedberg]

Pushed to m-c. http://hg.mozilla.org/mozilla-central/rev/54b5c634212e Appears to have stuck without any tinderbox orangeness

Comment 39

[Benjamin Smedberg]

Whoops. That was for a different bug.

Comment 40

[Daniel Holbert [:dholbert]]

Attachment: compilation errors after this landed in mozilla-central

It looks like this bug's fix was recently pushed as
  http://hg.mozilla.org/mozilla-central/rev/61ca56673468

However, this seems to break compilation of nsLayoutDebuggingTools.cpp (on my local Ubuntu machine, in my debug build) with errors including:

../../../dist/include/xpcom/nsISupportsUtils.h:203: error: ‘class nsIFrame’ has no member named ‘QueryInterface’
./../../dist/include/xpcom/nsISupportsUtils.h:203: error: no class template named ‘COMTypeInfo’ in ‘class nsIFrameDebug’

I'm attaching the relevant chunk of the compile log, for reference.

Perhaps this patch should be backed out?

Comment 41

[Benjamin Smedberg]

Is nsLayoutDebuggingTools not part of the default debug config? Porting should be straightforward and I'll have a patch up shortly.

Comment 42

[Daniel Holbert [:dholbert]]

Nope, I just realized it's not -- I think you need this in your .mozconfig:
   ac_add_options --enable-extensions=default,layout-debug

Comment 43

[Benjamin Smedberg]

Main patch:
http://hg.mozilla.org/mozilla-central/rev/61ca56673468

Static analysis and graphing code:
http://hg.mozilla.org/mozilla-central/rev/54cf0cbe42e8

Layout-debugger fixup:
http://hg.mozilla.org/mozilla-central/rev/52be68be1534

Except for a pre-existing windows crashtest failure, this stuck with no test failures.

Comment 44

[Serge Gautherie (:sgautherie)]

V.Fixed, per bug 391976 comment 6.

Comment 45

[Serge Gautherie (:sgautherie)]

Could this land on 1.9.1 branch too ? (To fix bug 458844 there too.)

Comment 46

[Benjamin Smedberg]

No.

Comment 47

[Eric Shepherd [:sheppy]]

This is mentioned here:

https://developer.mozilla.org/en/Firefox_3.6_for_developers#Other_interface_changes