Make nsIFrame not inherit from nsISupports

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Layout
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla1.9.2a1
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 280899 [details] [diff] [review]
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.
Attachment #280899 - Flags: superreview?(roc)
Attachment #280899 - Flags: review?(roc)
(Assignee)

Comment 1

11 years ago
Created attachment 280903 [details] [diff] [review]
Make nsIFrame not inherit from nsISupports, rev. 1.1

Whoops, forgot to hg qrefresh before uploading the patch
Attachment #280899 - Attachment is obsolete: true
Attachment #280899 - Flags: superreview?(roc)
Attachment #280899 - Flags: review?(roc)
(Assignee)

Updated

11 years ago
Attachment #280903 - Flags: superreview?(roc)
Attachment #280903 - Flags: review?(roc)
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.
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...
(Assignee)

Comment 4

11 years ago
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.
> 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.
Another thought... Would it be possible to have something more like:

  do_QueryInterface<nsIScrollableViewProvider> svp(scrollFrame);

The assignment operator could do the QI automatically.
(Assignee)

Comment 7

11 years ago
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.
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?
(Assignee)

Comment 9

11 years ago
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.
(see also bug 385843 about nsIPhonetic)
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...)
(Assignee)

Comment 12

11 years ago
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.
Call it mRawPtr at least. I'm used to it... I think it's worth not typing the type twice.
(Assignee)

Comment 14

11 years ago
Created attachment 281746 [details] [diff] [review]
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.
Attachment #280903 - Attachment is obsolete: true
Attachment #281746 - Flags: review?(roc)
Attachment #280903 - Flags: superreview?(roc)
Attachment #280903 - Flags: review?(roc)
What do you think about generalizing (the names of) nsFrameQI and nsFramePtr so that we can use them in other contexts?
(Assignee)

Comment 16

11 years ago
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?
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?
(Assignee)

Comment 18

11 years ago
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.
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?
(Assignee)

Comment 20

11 years ago
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.
Attachment #281746 - Flags: review?(roc)
(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.

Updated

11 years ago
OS: Mac OS X → All
Hardware: PC → All
Version: unspecified → Trunk
(Assignee)

Comment 22

11 years ago
Created attachment 282613 [details] [diff] [review]
Complete, all platforms, rev. 1

This is the completed patch with the jorendorff magic, ready for review (for mozilla-central)
Attachment #281746 - Attachment is obsolete: true
Attachment #282613 - Flags: review?(roc)
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.
(Assignee)

Comment 24

11 years ago
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.
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
(Assignee)

Comment 26

11 years ago
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.
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?
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
(Assignee)

Comment 29

11 years ago
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
(In reply to comment #30)
> Okay then, I accept that.

Whew! Spares us from DynamicCast. :-)

/be
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.
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.
(Assignee)

Updated

10 years ago
Depends on: 461186
(Assignee)

Updated

10 years ago
Attachment #282613 - Flags: review?(roc)
(Assignee)

Updated

10 years ago
Depends on: 461212
(Assignee)

Updated

10 years ago
Depends on: 461359
(Assignee)

Updated

10 years ago
Depends on: 461410
(Assignee)

Updated

10 years ago
Depends on: 461576
(Assignee)

Comment 34

10 years ago
Created attachment 344674 [details] [diff] [review]
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.
Attachment #282613 - Attachment is obsolete: true

Comment 35

10 years ago
>+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 ;-)

Updated

10 years ago
Depends on: 462203
(Assignee)

Comment 36

10 years ago
Created attachment 346532 [details] [diff] [review]
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.
Attachment #344674 - Attachment is obsolete: true
Attachment #346532 - Flags: superreview?(roc)
Attachment #346532 - Flags: review?(roc)
(Assignee)

Updated

10 years ago
Blocks: 391976
Attachment #346532 - Flags: superreview?(roc)
Attachment #346532 - Flags: superreview+
Attachment #346532 - Flags: review?(roc)
Attachment #346532 - Flags: review+
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.
(Assignee)

Comment 38

10 years ago
Pushed to m-c. http://hg.mozilla.org/mozilla-central/rev/54b5c634212e Appears to have stuck without any tinderbox orangeness
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 39

10 years ago
Whoops. That was for a different bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 356621 [details]
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?
(Assignee)

Comment 41

10 years ago
Is nsLayoutDebuggingTools not part of the default debug config? Porting should be straightforward and I'll have a patch up shortly.
Nope, I just realized it's not -- I think you need this in your .mozconfig:
   ac_add_options --enable-extensions=default,layout-debug
(Assignee)

Comment 43

10 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
V.Fixed, per bug 391976 comment 6.
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.2a1
Could this land on 1.9.1 branch too ? (To fix bug 458844 there too.)
Flags: wanted1.9.1?
(Assignee)

Comment 46

9 years ago
No.
(Assignee)

Updated

9 years ago
Flags: wanted1.9.1? → wanted1.9.1-
Keywords: dev-doc-needed
This is mentioned here:

https://developer.mozilla.org/en/Firefox_3.6_for_developers#Other_interface_changes
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.