Closed Bug 332648 Opened 18 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: bzbarsky, Assigned: Ms2ger)

References

Details

Attachments

(8 files)

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
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
(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: nobody → general
Component: DOM → JavaScript Engine
OS: Linux → All
QA Contact: general → general
Hardware: x86 → All
Blocks: 677079
Assignee: general → Ms2ger
Attachment #585707 - Flags: review?(evilpies)
Attachment #585712 - Flags: review?(evilpies)
Attachment #585713 - Flags: review?(evilpies)
(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+
(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.
(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.
You need to log in before you can comment on or make changes to this bug.