Closed
Bug 860591
Opened 12 years ago
Closed 12 years ago
"ABORT: what kind of wrapper is this?" with events
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla23
Tracking | Status | |
---|---|---|
firefox21 | --- | unaffected |
firefox22 | --- | fixed |
firefox23 | --- | fixed |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: jruderman, Assigned: bzbarsky)
References
Details
(Keywords: assertion, regression, testcase, Whiteboard: [adv-main22-])
Attachments
(5 files, 3 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
###!!! ABORT: what kind of wrapper is this?: 'IS_SLIM_WRAPPER(cur)', file js/xpconnect/src/XPCQuickStubs.cpp, line 633
Reporter | ||
Comment 1•12 years ago
|
||
Updated•12 years ago
|
Component: DOM: Events → XPConnect
Assignee | ||
Comment 2•12 years ago
|
||
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?
Flags: needinfo?(bugs)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #736296 -
Flags: review?(bugs)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → bzbarsky
Assignee | ||
Comment 4•12 years ago
|
||
This is the same except for removing the existing uievent and mouseevent quickstubs
Assignee | ||
Updated•12 years ago
|
Whiteboard: [need review]
Assignee | ||
Updated•12 years ago
|
Attachment #736303 -
Flags: review?(bugs)
Assignee | ||
Comment 5•12 years ago
|
||
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.
Attachment #736296 -
Flags: sec-approval?
Assignee | ||
Comment 6•12 years ago
|
||
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.
Attachment #736303 -
Flags: approval-mozilla-aurora?
Comment 7•12 years ago
|
||
> Olli, are there already some webidl events on Aurora 22?
Yes
status-b2g18:
--- → unaffected
status-firefox21:
--- → unaffected
status-firefox22:
--- → affected
status-firefox23:
--- → affected
status-firefox-esr17:
--- → unaffected
Comment 9•12 years ago
|
||
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?
Attachment #736296 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 10•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #736296 -
Flags: review?(bugs) → review+
Updated•12 years ago
|
Attachment #736303 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 11•12 years ago
|
||
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.
Assignee | ||
Comment 12•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #736296 -
Attachment is obsolete: true
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #736865 -
Flags: review?(bugs)
Assignee | ||
Comment 14•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #736303 -
Attachment is obsolete: true
Attachment #736303 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #736866 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Attachment #736865 -
Flags: review?(bugs) → review+
Comment 15•12 years ago
|
||
We'll approve once this sticks on m-c for a couple of days.
Assignee | ||
Comment 16•12 years ago
|
||
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.
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #736866 -
Attachment is obsolete: true
Attachment #736866 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #737073 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 18•12 years ago
|
||
Comment 19•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•12 years ago
|
Attachment #737073 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 20•12 years ago
|
||
Updated•11 years ago
|
Whiteboard: [adv-main22-]
Updated•11 years ago
|
Keywords: regression
Updated•11 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•