558235 - Kill CSS2PropertiesTearoff

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Zack Weinberg (:zwol)]

Attachment: patch

This is a follow-on to bug 508780.

Since there are no C++ users of nsIDOM(|SVG|NS)CSS2Properties, and since Javascript now consistently (well, almost) goes through special quickstubs for these interfaces that forward to nsICSSDeclaration::Get/SetPropertyValue, we ought to be able to eliminate CSS2PropertiesTearoff from the tree.

There are two wrinkles.  First, we *weren't* generating those special quickstubs for nsIDOMSVGCSS2Properties - that's a straightforward change to dom_quickstubs.qsconf.

More seriously, mochitests blow up in content/media with floods of errors like this:

43372 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_autobuffer.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - uncaught exception: [Exception... "Component does not have requested interface [nsIDOMCSS2Properties.display]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: chrome://global/content/bindings/videocontrols.xml :: anonymous :: line 656"  data: no] at :0

Line 656 of videocontrols.xml is just

                        // force style resolution, so that transition begins
                        // when we remove the attribute.
                        getComputedStyle(element, "").display;

The containing function can be called with 'element' equal to a variety of anonymous XUL elements injected by the binding.  I have no idea why this doesn't work; it's gotta be something to do with XBL and/or chromeness, because it works fine from straightforward non-chrome HTML and XUL test files.

Comment 1

[Zack Weinberg (:zwol)]

N.B. If it were felt to be useful, we could probably get more or less the same effect by having nsICSSDeclaration inherit directly from the various *CSS2Properties interfaces, but when I tried that I got a whole pile of ambiguous base errors and gave up.

Also, in addition to the XBL problem I described, I would really appreciate someone taking a close look at my changes to QueryInterface tables.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> I have no idea why this doesn't work; it's gotta be something to do with XBL
> and/or chromeness

The latter.  Specifically, since the running code is chrome (in the sense of the script URI being chrom://something) but is examining content objects (in the sense that their parent chain terminates at a content window) it gets XPCNativeWrapper wrappers around them, and property access on those is forced to go through XPConnect; that's the whole point of them.  So those accesses do not use quickstubs.

Your interface table stuff seems to have lost the offset table part of it; that's not entirely good.

Comment 3

[Peter Van der Beken [:peterv]]

Also, if we end up doing this (but that would mean solving the XPCNativeWrapper problem) we'll have to communicate about this, there might be C++ users outside our tree.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 438011 [details] [diff] [review]
patch

See comment 2 and 3.

Losing the offset table is not a problem I think. We currently use that only from quickstubs, but these quickstubs all use the bitmap-based fast unwrapping now (nsICSSDeclaration is one of the "castable" interfaces) so they don't even use the offset table anymore.

Comment 5

[Zack Weinberg (:zwol)]

(In reply to comment #2)
> > I have no idea why this doesn't work; it's gotta be something to do with XBL
> > and/or chromeness
> 
> The latter.  Specifically, since the running code is chrome (in the sense of
> the script URI being chrom://something) but is examining content objects (in
> the sense that their parent chain terminates at a content window) it gets
> XPCNativeWrapper wrappers around them, and property access on those is forced
> to go through XPConnect; that's the whole point of them.  So those accesses do
> not use quickstubs.

Bleah.  What can we do about it?

> Your interface table stuff seems to have lost the offset table part of it;
> that's not entirely good.

Clearly I'm missing something, but as far as I can tell, the _only_ difference between NS_OFFSET_AND_INTERFACE_TABLE and NS_INTERFACE_TABLE is the name of the QITableEntry array, and that variable is local to the QueryInterface() function that gets defined.  So it seems like it can't matter in practice.

(In reply to comment #3)
> Also, if we end up doing this (but that would mean solving the XPCNativeWrapper
> problem) we'll have to communicate about this, there might be C++ users outside
> our tree.

There shouldn't be any, it's an _IMPL_NS_LAYOUT class and it's hidden in libxul.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Bleah.  What can we do about it?

I'm not sure yet.

> Clearly I'm missing something

The difference between NS_OFFSET_AND_INTERFACE_TABLE_END and NS_INTERFACE_TABLE_END_WITH_PTR( (in particular the kThisPtrOffsetsSID stuff).

It still doesn't matter in this particular case per comment 4.

> There shouldn't be any

Why not?  The class is something we hand back when QIed to the nsIDOMCSS2Properties interface; they don't use it via CSS2PropertiesTearoff but rather through the nsIDOMCSS2Properties pointer and vtable.

Comment 7

[Zack Weinberg (:zwol)]

(In reply to comment #6)
> > Clearly I'm missing something
> 
> The difference between NS_OFFSET_AND_INTERFACE_TABLE_END and
> NS_INTERFACE_TABLE_END_WITH_PTR( (in particular the kThisPtrOffsetsSID stuff).

Oh.  Man.  That's really too much magic for my taste, but then, so are these macros in general.

> Why not?  The class is something we hand back when QIed to the
> nsIDOMCSS2Properties interface; they don't use it via CSS2PropertiesTearoff but
> rather through the nsIDOMCSS2Properties pointer and vtable.

Oh, good point.  That means I should maybe just go ahead and make nsCSSDeclaration/nsComputedDOMStyle implement the nsIDOM*CSS2Properties interfaces directly, assuming I can figure out the ambiguous nsISupports mess.  That would eliminate the XPCNativeWrapper problem as well.

Though it seems ... unfortunate ... that this kind of chrome-to-content access can't take advantage of quickstubs.

Comment 8

[Zack Weinberg (:zwol)]

Attachment: revised patch part 1: mechanical rename of conflicting methods in nsComputedDOMStyle

I managed to get past all of the ambiguous base problems, but it's kind of a mess and has negative side effects.  This preliminary patch renames a whole bunch of nsComputedDOMStyle methods that would hide nsIDOM*CSS2Properties methods with the same names (yes, despite the different argument list, bleah).

Comment 9

[Zack Weinberg (:zwol)]

Attachment: revised patch part 2: kill the tearoff, make nsICSSDeclaration inherit nsIDOMNSCSS2Properties (sort of)

And here's the meat of the change.

I had to introduce a shim class, nsICSSDeclarationWithProperties, because the quickstubs expect that nsISupports is an unambiguous base of nsICSSDeclaration.  Ugly, but I suppose we can live with it.

More seriously, nsDOMCSSDeclaration and its subclasses now have two vtable pointers.  I'm not sure this is avoidable since both nsIDOMCSSStyleDeclaration and nsIDOMCSS2Properties are frozen, and it makes me wonder if the tearoff isn't the lesser evil here.

It feels like there ought to be a way to avoid having nsComputedDOMStyle and nsDOMCSSAttributeDeclaration be cycle collection classes, but both them and their mContent can be directly referred to by javascript, so we may just be stuck, there.

Comment 10

[Zack Weinberg (:zwol)]

I forgot to mention the good news :) The revised two-part patch passes unit tests on the try server, 100%.

Comment 11

[Johnny Stenback (:jst)]

(In reply to comment #9)
> I had to introduce a shim class, nsICSSDeclarationWithProperties, because the
> quickstubs expect that nsISupports is an unambiguous base of nsICSSDeclaration.
>  Ugly, but I suppose we can live with it.

I don't think we have to live with it, we can certainly unfreeze these interfaces and merge them if that saves us a vtable and other pain here.

Comment 12

[Zack Weinberg (:zwol)]

A great deal of nastiness in this area could go away if nsIDOMCSSStyleDeclaration could be unfrozen, but I was under the impression that freezing was permanent.

Comment 13

[Johnny Stenback (:jst)]

Yes, freezing supposed to be permanent, but it has been decided that we will unfreeze interfaces where needed, and we certainly can't do all of electrolysis etc w/o unfreezing some interfaces, so doing that here is IMO an option.

Comment 14

[Zack Weinberg (:zwol)]

Attachment: revised patch part 2, alternative: with unfreezing

If I'm allowed to unfreeze things, this is what I'd do instead of the existing "part 2".  Do not be afraid of the hugeness - that's just because a whole lot of stuff moved from nsIDOM(SVG|NS)CSS2Properties.idl to nsIDOMCSS2Properties.idl.  It's actually less invasive than the previous alternative, except for the unfreezing part.  And it gets rid of the extra vtable in DOMCSSDeclarationImpl/nsComputedDOMStyle/nsDOMCSSAttributeDeclaration, so yay.

try server run in progress.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: patch part 2 alternative, v2a: with unfreezing

Forgot to 'hg add' the new .idl file.  Doh.

Comment 16

[Zack Weinberg (:zwol)]

Attachment: patch part 2 alternative, v3: with unfreezing

The try server caught a serious problem with the previous iteration of this patch, so here's a revision that fixes it.  I didn't bother to mention it last time around, but, I had to add an entirely new scriptable interface (nsIDOMCSSFontFaceDeclaration) for the object you get from the .style accessor on a @font-face rule's CSSOM object.  These were formerly nsIDOMCSSStyleDeclaration, but that type now has requirements on its C++ implementations that a @font-face declaration cannot satisfy.  The scriptable duck type is identical, so Javascript users will not notice the change.

The *fix* in this iteration of the patch is just to change a bunch more places on the C++ side that need to be aware of the change, lest any use of fontface.style throw NS_NOINTERFACE; most importantly, nsDOMClassInfo.  Fortunately, this stuff is covered by existing mochitests.

Comment 17

[Zack Weinberg (:zwol)]

It occurs to me that dbaron is as good a person to bother about this as bz, and he's said to be less busy just now.

Still interested in feedback from anyone and everyone on the general idea and the approaches taken.

Comment 18

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

I think the approach (having a single CSS2Properties shim that sits in the inheritance chain and adds an extra vtable pointer but no additional code relative to the current approach) seems like a good one.  (At least that's my understanding of it from skimming the patch.)

I didn't look into the "with unfreezing" patch; I don't see getting rid of an extra vtable pointer as that huge a benefit.  (Or does it have other advantages?)

I'm not a big fan of the names, though.

I think maybe the GetFoo -> QueryFoo rename would be better as GetFoo -> GetFooValue (though that might cause ambiguity given a CSS property ending in "Value") or maybe ValueOfFoo or ValueForFoo.

And I don't like the name nsIDOMCSSDeclarationWithProperties; it's not an interface, so it shouldn't start with nsI*.  You could perhaps just drop the "I" or the "IDOM".  But the important part of the class is that it provides the nsIDOMCSS2Properties implementation, so perhaps it should be nsCSS2Properties or nsCSS2PropertiesForDeclaration?

Comment 19

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

Comment on attachment 439963 [details] [diff] [review]
patch part 2 alternative, v3: with unfreezing

I'm inclined to say that we shouldn't take the unfreezing approach, although maybe there's some benefit that I'm missing.

Comment 20

[Zack Weinberg (:zwol)]

(In reply to comment #18)
> I think the approach (having a single CSS2Properties shim that sits in the
> inheritance chain and adds an extra vtable pointer but no additional code
> relative to the current approach) seems like a good one.  (At least that's my
> understanding of it from skimming the patch.)
> 
> I didn't look into the "with unfreezing" patch; I don't see getting rid of an
> extra vtable pointer as that huge a benefit.  (Or does it have other
> advantages?)

It's not that big a deal in itself, but presently nsDOMCSSDeclaration has no data members and one vtable pointer, so if we make it have two vtable pointers, its size doubles.  On a machine with 64-bit pointers, the concrete classes DOMCSSDeclarationImpl and nsDOMCSSAttributeDeclaration go from 16 to 24, and from 48 to 56 bytes, respectively.

> I'm not a big fan of the names, though.

Neither am I tbh.

> I think maybe the GetFoo -> QueryFoo rename would be better as GetFoo ->
> GetFooValue (though that might cause ambiguity given a CSS property ending in
> "Value") or maybe ValueOfFoo or ValueForFoo.

I'm not enthusiastic about those options either, but I think I'll go with ValueOfFoo.

> And I don't like the name nsIDOMCSSDeclarationWithProperties; it's not an
> interface, so it shouldn't start with nsI*.  You could perhaps just drop the
> "I" or the "IDOM".  But the important part of the class is that it provides the
> nsIDOMCSS2Properties implementation, so perhaps it should be nsCSS2Properties
> or nsCSS2PropertiesForDeclaration?

Do we have a naming convention for classes that are not "I" (in the sense of not being something you can QI for) but will never be instantiated directly?  I actually find the latter property more useful to code into the name than the former.

I might be able to arrange not to need this class at all, though, having learned a bit more about ambiguous bases while developing the "with unfreezing" patch.

Comment 21

[Zack Weinberg (:zwol)]

Er, I take that last assertion back.  We do need the shim class.  How does nsDOMCSSDeclarationAndProperties sound to you?

Comment 22

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

(In reply to comment #20)
> It's not that big a deal in itself, but presently nsDOMCSSDeclaration has no
> data members and one vtable pointer, so if we make it have two vtable pointers,
> its size doubles.  On a machine with 64-bit pointers, the concrete classes
> DOMCSSDeclarationImpl and nsDOMCSSAttributeDeclaration go from 16 to 24, and
> from 48 to 56 bytes, respectively.

I don't think that's the relevant comparison, since every one of these objects also has a bunch of other objects associated with it (nsCSSDeclaration, data block, etc.), and those are substantially larger.

(In reply to comment #21)
> Er, I take that last assertion back.  We do need the shim class.  How does
> nsDOMCSSDeclarationAndProperties sound to you?

I'd accept it (although I think I still prefer my last suggestion).

Comment 23

[Zack Weinberg (:zwol)]

Attachment: rv2 patch part 1: mechanical rename of conflicting methods

This is the same as the previous "revised patch part 1" but with "ValueOfXXX" methods instead of "QueryXXX" methods, as suggested.

Comment 24

[Zack Weinberg (:zwol)]

Attachment: rv2 patch part 2: kill CSS2PropertiesTearoff

And this is roughly the same as the original "revised part 2" without unfreezing, that dbaron liked, except without nsICSSDeclarationWithProperties; it turns out there *was* a way to make that work, viz. have nsDOMCSSDeclaration inherit from nsICSSDeclaration + nsINSCSS2Properties, then have nsComputedDOMStyle inherit from nsDOMCSSDeclaration.  This does mean nsComputedDOMStyle has to stub out some pure virtual methods declared by nsDOMCSSDeclaration but I think we can live with that.

It might be possible to fold nsICSSDeclaration into nsIDOMCSSStyleDeclaration or into nsDOMCSSDeclaration but I have not done that here.  Should probably get its own bug, if thought to be a good idea.

Comment 25

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

Comment on attachment 448918 [details] [diff] [review]
rv2 patch part 1: mechanical rename of conflicting methods

It occurs to me now that DoGet* rather than ValueOf* might have fit better with our existing habits, but this is ok too.

It would have been nice not to do line length/breaking cleanup in the same patch, but I reviewed both at the same time.

r=dbaron

Comment 26

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

Comment on attachment 448920 [details] [diff] [review]
rv2 patch part 2: kill CSS2PropertiesTearoff

Could you move the virtual functions that you define in
nsComputedDOMStyle.h into the .cpp file?

I'd prefer you leave the three #undef lines together in 
nsDOMCSSDeclaration.cpp

r=dbaron with that

Comment 27

[Zack Weinberg (:zwol)]

Attachment: part 1 as landed

I had to regenerate the renames because of code changes in the file (from bug 511339, bug 531334, bug 363249, etc.) so I made them DoGet* instead of ValueOf* per dbaron's musings above.  Carrying r+ forward.

Comment 28

[Zack Weinberg (:zwol)]

Attachment: part 2 as landed

(In reply to comment #26)
> Could you move the virtual functions that you define in
> nsComputedDOMStyle.h into the .cpp file?
> 
> I'd prefer you leave the three #undef lines together in 
> nsDOMCSSDeclaration.cpp
> 
> r=dbaron with that

Done and done.

Comment 29

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/e2346fa5faaa
http://hg.mozilla.org/mozilla-central/rev/e8f07df67a3e