692342 - Eliminate use of XPT_TDP_POINTER and XPT_TDP_REFERENCE in gecko

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Bobby Holley (:bholley)]

We have these flags for type descriptors in our typelib format, but there only ever seems to be one valid configuration for each type.

So about 90% of our usage of these flags ends up just being validation that the flags are what we think they should be. I'm reasonable confident that we can come up with a few macros that answer any questions we might have been trying to answer with these flags, and kill the flags from the typelib format.

Nuking these will put us in further violation of our own xpt spec, but I don't think anybody cares.

This will make life easier in bug 665409.

Comment 1

[Benjamin Smedberg]

I don't think we should be incompatibly changing the XPT file format unless we absolutely have to: it will be a royal PITA for a certain class of extension developer.

Comment 2

[Bobby Holley (:bholley)]

Just talked to bsmedberg and khuey about this on IRC.

There are really two changes to make here - removing gecko's dependency on these bits, and removing these bits from the typelib.

Doing the former won't break anything. Doing the latter might, and the reason we'd want to do that would be to use those bits for something else. We currently have one free bit, which I can use for bug 665409.

So for now, I'm going to only remove the _use_ of those bits from gecko, and leave them in the typelib. Then we forget about this for a while while we generate a good series of future-compatible gecko releases. We can come back to removing the bits from the typelib later. ;-)

Comment 3

[Bobby Holley (:bholley)]

16 patches on github: https://github.com/bholley/mozilla-central/tree/xpt_ptr

A lot of this is more or less a continuation of my refactoring work. XPCWrappedNative is starting to get pretty manageable! ;-)

Comment 4

[Bobby Holley (:bholley)]

Attachment: github URL (placeholder)

Flagging mrbkap for review.

On the off chance that anyone was looking at the branch before, please refresh - I just updated it a little bit.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 566374 [details]
github URL (placeholder)

This is going to need significant rebasing on top of bug 688012, so I'm canceling review for the time being.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 566374 [details]
github URL (placeholder)

This is now ready for mrbkap's review.

It applies on top of bug 677788, and passes the test coverage in bug 693341.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://github.com/bholley/mozilla-central/commit/98283373b2aea09f42bf009edcf1e79f2b649856
https://github.com/bholley/mozilla-central/commit/0c4b1804919c5f6c7124b39c5ce56d8a65793aec
https://github.com/bholley/mozilla-central/commit/319ea23acafc7d5800b92009a87d1bdfe9c0eb9a

These need some 'if(' -> 'if (' love.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Ms2ger from comment #7)
> https://github.com/bholley/mozilla-central/commit/
> 98283373b2aea09f42bf009edcf1e79f2b649856
> https://github.com/bholley/mozilla-central/commit/
> 0c4b1804919c5f6c7124b39c5ce56d8a65793aec
> https://github.com/bholley/mozilla-central/commit/
> 319ea23acafc7d5800b92009a87d1bdfe9c0eb9a
> 
> These need some 'if(' -> 'if (' love.

Should all be fixed now - thanks! :-)

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 566374 [details]
github URL (placeholder)

This looks pretty good. I haven't done the case analyses that you have, so I'm trusting you there. Overall, this looks like a really nice cleanup. I do have one question though:

-    // The xpidl compiler ensures this. We reaffirm it for safety.
-    if (arg_type.IsPointer() || arg_type.TagPart() != nsXPTType::T_U32)
-        return JS_FALSE;
+    // This should be enforced by the xpidl compiler, but it's not.
+    // See bug 695235.
+    NS_ABORT_IF_FALSE(arg_type.TagPart() == nsXPTType::T_U32,
+                      "size_is references parameter of invalid type.");

should this happen before this patch lands? It seems like it might be bad news if someone accidentally does this with no enforcement at all...

Comment 10

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #9)
> Comment on attachment 566374 [details]
> github URL (placeholder)
> 
> This looks pretty good. I haven't done the case analyses that you have, so
> I'm trusting you there. Overall, this looks like a really nice cleanup. I do
> have one question though:
> 
> -    // The xpidl compiler ensures this. We reaffirm it for safety.
> -    if (arg_type.IsPointer() || arg_type.TagPart() != nsXPTType::T_U32)
> -        return JS_FALSE;
> +    // This should be enforced by the xpidl compiler, but it's not.
> +    // See bug 695235.
> +    NS_ABORT_IF_FALSE(arg_type.TagPart() == nsXPTType::T_U32,
> +                      "size_is references parameter of invalid type.");
> 
> should this happen before this patch lands? It seems like it might be bad
> news if someone accidentally does this with no enforcement at all...

Well, we still have enforcement in debug builds thanks to the NS_ABORT_IF_FALSE, and I think it's pretty unlikely that anyone would be writing new IDL and never testing it in such a configuration. It'd certainly be enforced on tinderbox.

Still, it'd be good to get those enforcement bugs in sooner rather than later. Maybe khuey can be persuaded to do them? :-)

Comment 11

[Bobby Holley (:bholley)]

Landed to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0408a35a67f9
(and ancestors)

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c428312abbc7
https://hg.mozilla.org/mozilla-central/rev/42ab175e8d81
https://hg.mozilla.org/mozilla-central/rev/e8df70ae53e2
https://hg.mozilla.org/mozilla-central/rev/8790687685ee
https://hg.mozilla.org/mozilla-central/rev/4d417032d887
https://hg.mozilla.org/mozilla-central/rev/a03dd8e80c34
https://hg.mozilla.org/mozilla-central/rev/a8300adcea72
https://hg.mozilla.org/mozilla-central/rev/47047b832f46
https://hg.mozilla.org/mozilla-central/rev/206075dcdf40
https://hg.mozilla.org/mozilla-central/rev/74a834deb8a2
https://hg.mozilla.org/mozilla-central/rev/d7e55d8251a6
https://hg.mozilla.org/mozilla-central/rev/c1bc4c098814
https://hg.mozilla.org/mozilla-central/rev/2c2730b0cbf7
https://hg.mozilla.org/mozilla-central/rev/89e610a1014c
https://hg.mozilla.org/mozilla-central/rev/911074d770a2
https://hg.mozilla.org/mozilla-central/rev/0408a35a67f9

Comment 13

[(no longer active)]

This bug is a good example of why we should not do code reviews on github. :(

Comment 14

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #13)
> This bug is a good example of why we should not do code reviews on github. :(

Yeah. :-( Blake and I stopped doing that shortly after this, I think.

If you have any questions about this bug, I'm happy to answer them. :-)

Comment 15

[(no longer active)]

(In reply to comment #14)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #13)
> > This bug is a good example of why we should not do code reviews on github. :(
> 
> Yeah. :-( Blake and I stopped doing that shortly after this, I think.
> 
> If you have any questions about this bug, I'm happy to answer them. :-)

It's OK, I did have questions but figured them out with more hunting around.  That was just a rant.  :-)