781070 - make NullPtr public

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[:Benjamin Peterson]

The other rooting stuff works outside of JS by virture of being templates. NullPtr is concrete, though, so it needs JS_PUBLIC_API.

Comment 1

[:Benjamin Peterson]

Attachment: make it public

Comment 2

[Luke Wagner [:luke]]

Will this make normal use inside the JS engine slower (requiring reading the GOT to find &NullPtr::constNullValue)?

If so, one alternative is to have the NullPtr provide the NULL-holding memory location as a member variable, although this has hazards like:

  HandleObject h = NullPtr();
  // NullPtr() has been destroyed, h still points at address

Comment 3

[:Benjamin Peterson]

Luke and I talked about this and came to the conclusion that browser code can use Rooted* (cx, NULL) for now.

Comment 4

[Bobby Holley (:bholley)]

This makes me :-(

I don't think it would be acceptable to finalize a handle API without a convenient way for consumers to pass null. Given that, I think we might as well solve this problem now.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The handle API is not at all finalized. Where do you need this Bobby? Right now the only place where you might need to call a function that takes a handle is if you're calling a callback.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #5)
> The handle API is not at all finalized. Where do you need this Bobby? Right
> now the only place where you might need to call a function that takes a
> handle is if you're calling a callback.

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#1330

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bobby Holley (:bholley) from comment #6)
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.
> cpp#1330

Oh yeah. Those handles should not have been added. We decided to leave them in rather than convert them back to raw pointers. Maybe that was a mistake, though.

Comment 8

[Jeff Walden [:Waldo]]

Polyfilling std::nullptr_t per bug 806546 would also solve this problem, and probably better than by exposing NullPtr.  That said, I'm not totally sure it's possible to have both nullptr and std::nullptr_t at once, without compiler support, but I could easily be wrong.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #7)
> Oh yeah. Those handles should not have been added. We decided to leave them
> in rather than convert them back to raw pointers. Maybe that was a mistake,
> though.

Ah I see. I probably had an inflated view of how much the handles have leaked out into the DOM because I see them everywhere in XPConnect (with all the JSClass hooks and such). If I'm not supposed to be using them I'm happy to forget about this bug for the time being (so long as there is eventually a solution).

Comment 10

[Luke Wagner [:luke]]

When it does become relevant, we could add a JS_PUBLIC_API JS::NullHandle which would be separate from and thus avoid penalizing the engine-internal js::NullHandle.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] from comment #10)
> When it does become relevant, we could add a JS_PUBLIC_API JS::NullHandle
> which would be separate from and thus avoid penalizing the engine-internal
> js::NullHandle.

sgtm.

Comment 12

[Jon Coppeard (:jonco)]

Attachment: Introduce separate public JS::NullPtr

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 726677 [details] [diff] [review]
Introduce separate public JS::NullPtr

Review of attachment 726677 [details] [diff] [review]:
-----------------------------------------------------------------

Nicely done!

::: js/public/RootingAPI.h
@@ +118,5 @@
>  template <typename T>
>  class MutableHandleBase {};
>  
> +/*
> + * Pass a NULL handle, internal version.

Lets expand on this a bit to give more context. Something like:

"js::NullPtr acts like a NULL pointer in contexts that require a Handle. This is the SpiderMonkey internal variant. js::NullPtr should be used in preference to JS::NullPtr to avoid the GOT access required for JS_PUBLIC_API symbols."

@@ +149,5 @@
>  CheckStackRoots(JSContext *cx);
>  #endif
>  
>  /*
> + * Pass a NULL handle, public version.

The same comment as above, but we can stop after the first sentence.

Comment 14

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74b7f2c42ca4

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/74b7f2c42ca4