Last Comment Bug 761707 - Use the same slot index for the "self" pointer for global and non-global objects
: Use the same slot index for the "self" pointer for global and non-global objects
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: ParisBindings
  Show dependency treegraph
 
Reported: 2012-06-05 10:50 PDT by Boris Zbarsky [:bz]
Modified: 2012-06-09 19:17 PDT (History)
3 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1. Rip out the various infrastructure for allowing different slot indices on different DOMJSClass instances. (7.06 KB, patch)
2012-06-05 11:42 PDT, Boris Zbarsky [:bz]
bobbyholley: review+
Details | Diff | Splinter Review
part 2. Drop the vestigial jsclass argument to UnwrapDOMObject. (16.57 KB, patch)
2012-06-05 11:42 PDT, Boris Zbarsky [:bz]
bobbyholley: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-06-05 10:50:00 PDT
Slot 0 is safe for globals, even though it's technically reserved right this second.  So we should just use it.
Comment 1 Boris Zbarsky [:bz] 2012-06-05 11:42:01 PDT
Created attachment 630243 [details] [diff] [review]
part 1.  Rip out the various infrastructure for allowing different slot indices on different DOMJSClass instances.
Comment 2 Boris Zbarsky [:bz] 2012-06-05 11:42:19 PDT
Created attachment 630244 [details] [diff] [review]
part 2.  Drop the vestigial jsclass argument to UnwrapDOMObject.
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-06-06 02:40:22 PDT
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?
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2012-06-06 02:52:56 PDT
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?
Comment 5 Boris Zbarsky [:bz] 2012-06-06 07:59:42 PDT
> 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 Bobby Holley (:bholley) (busy with Stylo) 2012-06-06 08:10:52 PDT
(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 9 :Ms2ger (⌚ UTC+1/+2) 2012-06-07 07:51:29 PDT
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?
Comment 10 Boris Zbarsky [:bz] 2012-06-07 07:52:11 PDT
> Shouldn't we keep asserting that IsDOMClass(JS_GetClass(obj)) is true?

Hmm.  That's a good point.  I'll readd that.
Comment 11 Boris Zbarsky [:bz] 2012-06-08 17:38:17 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7b8279ce16a readds the assert.
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-06-09 19:17:53 PDT
https://hg.mozilla.org/mozilla-central/rev/a7b8279ce16a

Note You need to log in before you can comment on or make changes to this bug.