Expose js::AutoGCRooter and its subclasses via a public header

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: bz, Assigned: Ms2ger)

Tracking

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments)

(Reporter)

Description

12 years ago
See bug 328007 comment 2.

What approach should we be using when we have a JSContext*?  I believe pretty much all consumers of nsAutoGCRoot have one, so if there's something better we could be doing, we should do it.
It's private or at best "friend" API, but you could include "jscntxt.h" and use JSTempValueRooter, JS_PUSH_SINGLE_TEMP_ROOT, and JS_POP_TEMP_ROOT.  We are all friends here, eh?

/be
If an array of jsvals needs protecting, use JS_PUSH_TEMP_VALUE_ROOT.

/be
(Reporter)

Comment 3

11 years ago
Actually, I'd appreciate either docs on this stuff or someone else doing the patch; I can't easily make sense of the API in jscntxt.h...
Assignee: general → nobody
QA Contact: ian → general
There's still a FIXME comment in jscntxt.h pointing here. Changing the summary to reflect where we are now.

This is still wanted. Really the main motivation is not to make to make js::AutoGCRooter and so forth public but to make jscntxt.h private again. Anyone writing serious JSAPI code is basically forced to include it; these classes are just too useful to ignore.

One way to fix this is:

    namespace js {

    struct ExposedContextFields {
        volatile jsint operationCallbackFlag;
        AutoGCRooter   *autoGCRooters;
    };

    class AutoGCRooter {
      public:
        AutoGCRooter(JSContext *cx, ptrdiff_t tag)
          : context((ExposedContextFields *) cx), down(context->autoGCRooters),
            tag(tag)
        {
            context->autoGCRooters = this;
        }
        ...
    };

    } /* namespace js */

Then in jscntxt.h we arrange for JSContext's layout to be just so. (I don't think it can just be a derived class, because of JSD, but I might be wrong.)

I wonder if jwalden would be willing to do the honors.
Summary: Do better GC safety in nsAutoGCRoot if we have a JSContext* → Expose js::AutoGCRooter and its subclasses via a public header
Hmm. Actually, if we're as close on bug 516832 as it looks, maybe we can WONTFIX this soon.
(In reply to comment #5)
> Hmm. Actually, if we're as close on bug 516832 as it looks, maybe we can
> WONTFIX this soon.

We still need temporary rooting for values and value vectors not on the thread stack, even if that bug comes in quickly. Will it? I can't tell. But we'll still need fast RAII helpers for some hard cases, so why not fix this bug too/first?

/be

Comment 7

7 years ago
(In reply to comment #5)
> Hmm. Actually, if we're as close on bug 516832 as it looks, maybe we can
> WONTFIX this soon.

We may need some time to have the conservative stack scanner working only in an assertion mode to verify a scope of potential leaks. Still that should be done quickly so I agree with WONTFIX here. We may expose some extra classes to do the rooting of value arrays etc., but that do not need inline methods.
(Assignee)

Updated

6 years ago
Assignee: nobody → general
Component: DOM → JavaScript Engine
OS: Linux → All
QA Contact: general → general
Hardware: x86 → All
(Assignee)

Updated

6 years ago
Blocks: 677079
(Assignee)

Updated

6 years ago
Assignee: general → Ms2ger
(Assignee)

Comment 8

6 years ago
Created attachment 585707 [details] [diff] [review]
Part a: Move AutoGCRooter to jsapi.h.
Attachment #585707 - Flags: review?(evilpies)
(Assignee)

Comment 9

6 years ago
Created attachment 585708 [details] [diff] [review]
Part b: Move AutoValueRooter to jsapi.h.
Attachment #585708 - Flags: review?(evilpies)
(Assignee)

Comment 10

6 years ago
Created attachment 585709 [details] [diff] [review]
Part c: Move AutoObjectRooter to jsapi.h.
Attachment #585709 - Flags: review?(evilpies)
(Assignee)

Comment 11

6 years ago
Created attachment 585710 [details] [diff] [review]
Part d: Move AutoStringRooter to jsapi.h.
Attachment #585710 - Flags: review?(evilpies)
(Assignee)

Comment 12

6 years ago
Created attachment 585711 [details] [diff] [review]
Part e: Move AutoArrayRooter to jsapi.h.
Attachment #585711 - Flags: review?(evilpies)
(Assignee)

Comment 13

6 years ago
Created attachment 585712 [details] [diff] [review]
Part f: Move AutoIdRooter to jsapi.h.
Attachment #585712 - Flags: review?(evilpies)
(Assignee)

Comment 14

6 years ago
Created attachment 585713 [details] [diff] [review]
Part g: Move AutoIdArray to jsapi.h.
Attachment #585713 - Flags: review?(evilpies)
(Assignee)

Comment 15

6 years ago
Created attachment 585714 [details] [diff] [review]
Part h: Move AutoEnumStateArray to jsapi.h.
Attachment #585714 - Flags: review?(evilpies)

Comment 16

6 years ago
(In reply to Ms2ger from comment #8)
> Created attachment 585707 [details] [diff] [review]
> Part a: Move AutoGCRooter to jsapi.h.

Is the reason for this is the exact GC work that is going on? If so, why not to base the rooters on Handle and other classes?
Attachment #585708 - Flags: review?(evilpies) → review+
Attachment #585709 - Flags: review?(evilpies) → review+
Comment on attachment 585710 [details] [diff] [review]
Part d: Move AutoStringRooter to jsapi.h.

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

::: js/src/jscntxt.h
@@ -1602,5 @@
>  typedef RootedVar<JSAtom*>            RootedVarAtom;
>  typedef RootedVar<jsid>               RootedVarId;
>  typedef RootedVar<Value>              RootedVarValue;
>  
>  /* FIXME(bug 332648): Move this into a public header. */

Remove?
Attachment #585710 - Flags: review?(evilpies) → review+
Attachment #585711 - Flags: review?(evilpies) → review+
Comment on attachment 585712 [details] [diff] [review]
Part f: Move AutoIdRooter to jsapi.h.

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

::: js/src/jsgc.cpp
@@ +1995,5 @@
>              MarkRoot(trc, obj, "js::AutoObjectRooter.obj");
>          return;
>  
>        case ID:
> +        MarkRoot(trc, static_cast<AutoIdRooter *>(this)->id_, "js::AutoIdRooter.id_");

Nit: JS::AutoIdRooter.id_ ?
Attachment #585712 - Flags: review?(evilpies) → review+
Comment on attachment 585713 [details] [diff] [review]
Part g: Move AutoIdArray to jsapi.h.

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

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +517,5 @@
>              return NS_ERROR_FAILURE;
>            }
>  
>            if (JSVAL_IS_BOOLEAN(propval)) {
> +            contextProps->SetPropertyAsBool(pstr, JSVAL_TO_BOOLEAN(propval));

Thanks for these changes in this file :O
Attachment #585713 - Flags: review?(evilpies) → review+
Comment on attachment 585714 [details] [diff] [review]
Part h: Move AutoEnumStateArray to jsapi.h.

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

::: js/src/jscntxt.cpp
@@ +1742,5 @@
>          JS_ASSERT(ok);
>      }
>  }
>  
> +} /* namespace JS */

I am not sure, but why isn't this is jsapi.cpp ?
Attachment #585714 - Flags: review?(evilpies) → review+
Comment on attachment 585707 [details] [diff] [review]
Part a: Move AutoGCRooter to jsapi.h.

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

::: js/src/jsapi.cpp
@@ +6594,5 @@
> +SetAutoGCRooter(JSContext *cx, AutoGCRooter *rooter)
> +{
> +    cx->autoGCRooters = rooter;
> +}
> +

As on IRC, we shouldn't expose them.
Attachment #585707 - Flags: review?(evilpies) → review+
(Assignee)

Comment 22

6 years ago
(In reply to Igor Bukanov from comment #16)
> (In reply to Ms2ger from comment #8)
> > Created attachment 585707 [details] [diff] [review]
> > Part a: Move AutoGCRooter to jsapi.h.
> 
> Is the reason for this is the exact GC work that is going on? If so, why not
> to base the rooters on Handle and other classes?

The reason is that Gecko uses these classes, and we don't want Gecko to include jscntxt.h. If there are better APIs Gecko is supposed to use, I'd be happy to see patches to that effect, but I don't know enough about what this code does to do that myself and still sleep well at night.

Comment 23

6 years ago
(In reply to Ms2ger from comment #22)
> (In reply to Igor Bukanov from comment #16)
> > (In reply to Ms2ger from comment #8)
> > > Created attachment 585707 [details] [diff] [review]
> > > Part a: Move AutoGCRooter to jsapi.h.
> > 
> > Is the reason for this is the exact GC work that is going on? If so, why not
> > to base the rooters on Handle and other classes?
> 
> The reason is that Gecko uses these classes, and we don't want Gecko to
> include jscntxt.h. If there are better APIs Gecko is supposed to use, I'd be
> happy to see patches to that effect, but I don't know enough about what this
> code does to do that myself and still sleep well at night.

If this is only for Gecko, move the classes to jsfriendapi.h, not jsapi.h. Also lets not add those GetAutoGCRooter and SetAutoGCRooter as they introduce a couple of calls per each rotter instance. Instead just add a class like JSBaseContext that contains the fields that are necessary to implement the rooting and then subclass JSContext from it. This way the rooters can just cast the passed JSContext instance to JSBaseContext and access the rooter stack.
(Assignee)

Comment 24

6 years ago
https://hg.mozilla.org/mozilla-central/rev/4c85015dc460
https://hg.mozilla.org/mozilla-central/rev/6407283df40b
https://hg.mozilla.org/mozilla-central/rev/422290830373
https://hg.mozilla.org/mozilla-central/rev/13f996c0d905
https://hg.mozilla.org/mozilla-central/rev/af8168703a40
https://hg.mozilla.org/mozilla-central/rev/8fd1b8345680
https://hg.mozilla.org/mozilla-central/rev/2140f9cb5d6a
https://hg.mozilla.org/mozilla-central/rev/4fc86339a424
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.