Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Use the same slot index for the "self" pointer for global and non-global objects

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

unspecified
mozilla16
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Slot 0 is safe for globals, even though it's technically reserved right this second.  So we should just use it.
(Assignee)

Comment 1

5 years ago
Created attachment 630243 [details] [diff] [review]
part 1.  Rip out the various infrastructure for allowing different slot indices on different DOMJSClass instances.
Attachment #630243 - Flags: review?(bobbyholley+bmo)
(Assignee)

Comment 2

5 years ago
Created attachment 630244 [details] [diff] [review]
part 2.  Drop the vestigial jsclass argument to UnwrapDOMObject.
Attachment #630244 - Flags: review?(bobbyholley+bmo)
(Assignee)

Updated

5 years ago
Assignee: nobody → bzbarsky
Blocks: 580070
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+
(Assignee)

Comment 5

5 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.
(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

5 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

5 years ago
https://hg.mozilla.org/mozilla-central/rev/d15b364be513
https://hg.mozilla.org/mozilla-central/rev/1a2c4e651e30
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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

5 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

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7b8279ce16a readds the assert.
https://hg.mozilla.org/mozilla-central/rev/a7b8279ce16a
You need to log in before you can comment on or make changes to this bug.