Last Comment Bug 695235 - XPIDL compiler should enforce size_is parameters being T_U32
: XPIDL compiler should enforce size_is parameters being T_U32
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Nathan Froyd [:froydnj]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-17 17:05 PDT by Bobby Holley (busy)
Modified: 2012-03-14 05:20 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.73 KB, patch)
2012-03-09 14:08 PST, Nathan Froyd [:froydnj]
khuey: review+
Details | Diff | Review

Description Bobby Holley (busy) 2011-10-17 17:05:30 PDT
There's a comment in XPConnect saying that the XPIDL compiler enforces this, but pyxpidl doesn't seem to.

The checking we do for this in xpconnect is actually going away in bug 692342, which will land real soon now. So I think it would be very good to get this fixed. Otherwise, we'll have nothing to warn people that they're doing the wrong thing.
Comment 1 Nathan Froyd [:froydnj] 2012-03-09 14:08:24 PST
Created attachment 604525 [details] [diff] [review]
patch

Check size_is parameter type and also sanity check that the parameter exists.

Surprisingly (?), no problems on Linux/x86-64 with these checks.
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-11 15:43:50 PDT
Comment on attachment 604525 [details] [diff] [review]
patch

I would prefer that the check went into Param.__init__ to avoid having to loop through all the params.
Comment 3 Nathan Froyd [:froydnj] 2012-03-11 15:58:32 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> I would prefer that the check went into Param.__init__ to avoid having to
> loop through all the params.

You can't do that, though, can you?  Because for:

method(... [array, size_is(count)] vector, count...)

When you're __init__'ing count, you don't know that it's a size_is parameter, and when you're __init__'ing vector, you don't have access to the type of count.  So you have to do it in Method...I think.  (Checking for the existence of the size_is parameter requires looping anyway.)
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-11 16:01:04 PDT
mmm, right.  That's what I get for forgetting how this works.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-11 16:01:23 PDT
Comment on attachment 604525 [details] [diff] [review]
patch

Review of attachment 604525 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this is fine.
Comment 6 Mozilla RelEng Bot 2012-03-11 16:59:04 PDT
Autoland Patchset:
	Patches: 604525
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=5f36c384197d
Try run started, revision 5f36c384197d. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=5f36c384197d
Comment 7 Mozilla RelEng Bot 2012-03-11 20:46:52 PDT
Try run for 5f36c384197d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5f36c384197d
Results (out of 14 total builds):
    success: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-5f36c384197d
Comment 9 Marco Bonardo [::mak] 2012-03-13 04:40:44 PDT
https://hg.mozilla.org/mozilla-central/rev/c2d251ee72d5
Comment 10 :Ms2ger 2012-03-14 01:20:50 PDT
Shouldn't the error message say size_is instead of is_size?
Comment 11 Nathan Froyd [:froydnj] 2012-03-14 05:20:46 PDT
(In reply to Ms2ger from comment #10)
> Shouldn't the error message say size_is instead of is_size?

Doh.  Yes, in both places.

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