860591 - "ABORT: what kind of wrapper is this?" with events

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ABORT: what kind of wrapper is this?: 'IS_SLIM_WRAPPER(cur)', file js/xpconnect/src/XPCQuickStubs.cpp, line 633

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

So unsurprisingly "cur" is a WebIDL object.

The code looks like this:

628             if (NS_SUCCEEDED(getNative(native, entries, cur, iid, ppThis, pThisRef, vp))) {
629                 if (lccx) {
630                     // This only matters for unwrapping of this objects, so we
631                     // shouldn't end up here for the new DOM bindings.
632                     NS_ABORT_IF_FALSE(IS_SLIM_WRAPPER(cur),
633                                       "what kind of wrapper is this?");
634                     lccx->SetWrapper(cur);
635                 }

It seems to me that that comment is obviously wrong.  I'm a little surprised we never hit this before...  I guess in the past we always got rid of quicktubs for the shared interfaces (replacing them with new-bindings methods on all the protos) before we started converting some instances.  We thought of it as an optimization, but it was actually a necessary correctness fix!

Can we do that for events?  Olli, are there already some webidl events on Aurora 22?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Install WebIDL quickstubs for event interfaces as needed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Aurora version

This is the same except for removing the existing uievent and mouseevent quickstubs

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 736296 [details] [diff] [review]
Install WebIDL quickstubs for event interfaces as needed.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Unclear.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The tests are pretty clear.

Which older supported branches are affected by this flaw? Aurora 22.

If not all supported branches, which bug introduced the flaw?  Bug 822399.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Aurora backport is attached.

How likely is this patch to cause regressions; how much testing does it need?  Risk is medium; I'd like to get as much testing as I can.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 736303 [details] [diff] [review]
Aurora version

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 822399 
User impact if declined: Invariant violation with unclear consequences
Testing completed (on m-c, etc.): Passes attached tests.
Risk to taking this patch (and alternatives if risky): Medium risk; this stuff is
  fiddly.  The main alternative is turning off WebIDL bindings for events on
  Aurora, but that may be riskier.
String or IDL/UUID changes made by this patch:  None.

Comment 7

[Andrew McCreight [:mccr8]]

> Olli, are there already some webidl events on Aurora 22?
Yes

Comment 8

[Andrew McCreight [:mccr8]]

(see the various blocking bugs on bug 776864)

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 736296 [details] [diff] [review]
Install WebIDL quickstubs for event interfaces as needed.

sec-approval+.

Can you suggest a security rating for this issue?

Comment 10

[Boris Zbarsky [:bzbarsky]]

I have no idea, actually.  I'm not sure whether the lccx actually depends on the object being an XPConnect wrapper and if so what will go wrong in an opt build if it's not.

Comment 11

[Boris Zbarsky [:bzbarsky]]

This was pushed in https://hg.mozilla.org/integration/mozilla-inbound/rev/8488f69f8f91 and then backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1f956ddfa7a4 because those base classes the patch uses are totally wrong.  I'll fix the patch.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to fix that issue

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for review delectation

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated Aurora patch

Comment 15

[Alex Keybl [:akeybl]]

We'll approve once this sticks on m-c for a couple of days.

Comment 16

[Boris Zbarsky [:bzbarsky]]

So I pushed this to try, and there is one test "failure":

12:14:30 INFO - 9135 ERROR TEST-UNEXPECTED-PASS | /tests/dom/imptests/webapps/DOMCore/tests/approved/test_interfaces.html | Event interface: calling initEvent(DOMString,boolean,boolean) on new CustomEvent("foo") with too few arguments must throw TypeError

I'm removing the relevant line from dom/imptests/failures/webapps/DOMCore/tests/approved/test_interfaces.html.json before I push this.  Will post an Aurora patch that includes that too.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Even more updated aurora patch

Comment 18

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f911913dadf

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4f911913dadf

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bb36cb83def8