Last Comment Bug 692342 - Eliminate use of XPT_TDP_POINTER and XPT_TDP_REFERENCE in gecko
: Eliminate use of XPT_TDP_POINTER and XPT_TDP_REFERENCE in gecko
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on: 705875
Blocks: 665409 693454 708499 712858
  Show dependency treegraph
 
Reported: 2011-10-05 18:25 PDT by Bobby Holley (busy)
Modified: 2014-04-28 15:30 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
github URL (placeholder) (58 bytes, text/plain)
2011-10-11 15:34 PDT, Bobby Holley (busy)
mrbkap: review+
Details

Description Bobby Holley (busy) 2011-10-05 18:25:07 PDT
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 [:bsmedberg] 2011-10-06 04:09:06 PDT
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 (busy) 2011-10-06 09:50:53 PDT
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 (busy) 2011-10-10 11:07:26 PDT
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 (busy) 2011-10-11 15:34:11 PDT
Created attachment 566374 [details]
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 (busy) 2011-10-13 15:01:45 PDT
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 (busy) 2011-10-20 13:52:31 PDT
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 8 Bobby Holley (busy) 2011-10-25 10:13:06 PDT
(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) (please use needinfo!) 2011-11-25 08:24:46 PST
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 (busy) 2011-11-25 09:42:21 PST
(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 (busy) 2011-11-25 17:14:48 PST
Landed to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0408a35a67f9
(and ancestors)
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2014-04-27 07:31:30 PDT
This bug is a good example of why we should not do code reviews on github. :(
Comment 14 Bobby Holley (busy) 2014-04-28 13:41:35 PDT
(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 :Ehsan Akhgari (busy, don't ask for review please) 2014-04-28 15:30:41 PDT
(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.  :-)

Note You need to log in before you can comment on or make changes to this bug.