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)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Slot 0 is safe for globals, even though it's technically reserved right this second.  So we should just use it.
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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 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+
> 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.
(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. :-)
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?
> Shouldn't we keep asserting that IsDOMClass(JS_GetClass(obj)) is true?

Hmm.  That's a good point.  I'll readd that.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: