Closed
Bug 761707
Opened 12 years ago
Closed 12 years ago
Use the same slot index for the "self" pointer for global and non-global objects
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
7.06 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
16.57 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
Slot 0 is safe for globals, even though it's technically reserved right this second. So we should just use it.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #630243 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #630244 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Updated•12 years ago
|
Comment 3•12 years ago
|
||
Comment on attachment 630243 [details] [diff] [review] part 1. Rip out the various infrastructure for allowing different slot indices on different DOMJSClass instances. >- -1, false, DOM_OBJECT_SLOT >+ -1, false, NULL >- -1, false, DOM_GLOBAL_OBJECT_SLOT >+ -1, false, NULL I assume it was an accident that mNativeHooks was previous unspecified here? It's guaranteed to be zero-ed anyway, right?
Attachment #630243 -
Flags: review?(bobbyholley+bmo) → review+
Comment 4•12 years ago
|
||
Comment on attachment 630244 [details] [diff] [review] part 2. Drop the vestigial jsclass argument to UnwrapDOMObject. > QueryInterface(JSContext* cx, unsigned argc, JS::Value* vp) > { > JS::Value thisv = JS_THIS(cx, vp); > if (thisv == JSVAL_NULL) > return false; > > JSObject* obj = JSVAL_TO_OBJECT(thisv); > JSClass* clasp = js::GetObjectJSClass(obj); >- if (!IsDOMClass(clasp)) { >+ if (!IsDOMClass(clasp) || >+ !DOMJSClass::FromJSClass(clasp)->mDOMObjectIsISupports) { > return Throw<true>(cx, NS_ERROR_FAILURE); > } > >- nsISupports* native = UnwrapDOMObject<nsISupports>(obj, clasp); >+ nsISupports* native = UnwrapDOMObject<nsISupports>(obj); I assume you've checked to ensure that we only get here if mDOMObjectIsISupports is true?
Attachment #630244 -
Flags: review?(bobbyholley+bmo) → review+
Assignee | ||
Comment 5•12 years ago
|
||
> I assume it was an accident that mNativeHooks was previous unspecified here? Indeed. When we added the field to the JSClass no one updated the worker code accordingly, and it all worked because workers never use that field. > I assume you've checked to ensure that we only get here if mDOMObjectIsISupports is true? Er... We don't, in general, which is why I added that check. But if people aren't trying to break stuff, then yes: QueryInterface is only exposed on things that are nsISupports. I added the check in case someone grabs it off there and .call()s on a different DOM object.
Comment 6•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #5) > Er... We don't, in general, which is why I added that check. Oh, whoops! I misread the diff, and thought you were removing that check rather than adding it. :-)
Assignee | ||
Comment 7•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d15b364be513 https://hg.mozilla.org/integration/mozilla-inbound/rev/1a2c4e651e30
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla16
Comment 8•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d15b364be513 https://hg.mozilla.org/mozilla-central/rev/1a2c4e651e30
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 9•12 years ago
|
||
Comment on attachment 630243 [details] [diff] [review] part 1. Rip out the various infrastructure for allowing different slot indices on different DOMJSClass instances. Review of attachment 630243 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/BindingUtils.h @@ -64,5 @@ > inline T* > UnwrapDOMObject(JSObject* obj, const JSClass* clasp) > { > - MOZ_ASSERT(IsDOMClass(clasp)); > - MOZ_ASSERT(JS_GetClass(obj) == clasp); Shouldn't we keep asserting that IsDOMClass(JS_GetClass(obj)) is true?
Assignee | ||
Comment 10•12 years ago
|
||
> Shouldn't we keep asserting that IsDOMClass(JS_GetClass(obj)) is true?
Hmm. That's a good point. I'll readd that.
Assignee | ||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7b8279ce16a readds the assert.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•