XPIDL compiler should enforce size_is parameters being T_U32

RESOLVED FIXED in mozilla13

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bholley, Assigned: froydnj)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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

Comment 1

6 years ago
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.
Attachment #604525 - Flags: review?(khuey)
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.
Attachment #604525 - Flags: review?(khuey) → review-
(Assignee)

Comment 3

6 years ago
(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.)
mmm, right.  That's what I get for forgetting how this works.
Comment on attachment 604525 [details] [diff] [review]
patch

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

Ok, this is fine.
Attachment #604525 - Flags: review- → review+
(Assignee)

Updated

6 years ago
Whiteboard: [autoland-try:-b do -p all -u none -t none]

Updated

6 years ago
Whiteboard: [autoland-try:-b do -p all -u none -t none] → [autoland-in-queue]

Comment 6

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

Updated

6 years ago
Keywords: checkin-needed

Comment 7

6 years ago
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

Updated

6 years ago
Whiteboard: [autoland-in-queue]
http://hg.mozilla.org/integration/mozilla-inbound/rev/c2d251ee72d5
Keywords: checkin-needed
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/c2d251ee72d5
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Shouldn't the error message say size_is instead of is_size?
Assignee: khuey → nfroyd
(Assignee)

Comment 11

6 years ago
(In reply to Ms2ger from comment #10)
> Shouldn't the error message say size_is instead of is_size?

Doh.  Yes, in both places.
You need to log in before you can comment on or make changes to this bug.