GC protection of jsid
Benjamin Smedberg
https://bugzilla.mozilla.org/show_bug.cgi?id=653083 where JS IDs are
being cached in ways that are apparently not safe. We have a type
"NPIdentifier" which is basically a wrapper around jsid. Plugins treat
NPIdentifiers as permanent identities (in most cases, at least).
In some cases, the identifier is obtained directly by calling
NPN_GetStringIdentifier, which ends up calling JS_InternUCStringN
at
http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp#728
and then INTERNED_STRING_TO_JSID here
http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.h#148
But when jsids are passed in through hooks such as JSClass.getProperty,
the resulting NPIdentifier is only valid in the current call. This
breaks our cache mechanism for multi-process plugins here:
http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginModuleParent.cpp#602
jorendorff said in #jsapi:
> <jorendorff> bsmedberg: OK, you might want to post on js-engine, but
> here is my two-line answer...
> <jorendorff> [1] Going "by the book", you'd probably have to pass
> that jsid through JS_IdToValue and then JS_ValueToString and
> JS_GetStringCharsAndLength and JS_InternUCString to get something
> really permanent
> <jorendorff> [2] We can make an api that does exactly what you want
> for cheap
> * jorendorff notes that if you do things by the book, hours seem like days
> <bsmedberg> heh
> <jorendorff> bsmedberg: the reason for [2] is that you can literally
> just set some bits on the string that say "don't gc me" (and sprinkle
> appropriate assertions)
> <jorendorff> (if you're inside the engine, that is -- these flags are
> not part of the JSAPI)
> <bent> jorendorff, bsmedberg, couldn't you just do:
> <bent> if (JSID_IS_STRING(aID)) {
> <bent> JSString* str = JS_InternJSString(aCx, JSID_TO_STRING(aId));
> <bent> aID = INTERNED_STRING_TO_JSID(str);
> <bent> }
> <jorendorff> bent: yes
> * jorendorff forgot about JS_InternJSString
Should I manually intern the jsid-as-string using bent's mechanic? Or
should I be doing this differently somehow?
--BDS
Benjamin Smedberg
> But when jsids are passed in through hooks such as
> JSClass.getProperty, the resulting NPIdentifier is only valid in the
> current call. This breaks our cache mechanism for multi-process
> plugins here:
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginModuleParent.cpp#602
non-interned string IDs. I don't think this is possible with the current
JSAPI because it doesn't expose the intern-edness of a jsid. Adding a
JS_IDIsPermanent API would make this fix fairly straightforward.
--BDS
Jason Orendorff
> But when jsids are passed in through hooks such as JSClass.getProperty,
> the resulting NPIdentifier is only valid in the current call.
There are at least three issues here:
- jsid GC-safety
- jsid hashability and equality
- bugs
Chris Leary is working the bugs; hashability and equality can wait a day
or two as long as you don't forget about it. That leaves GC-safety.
If plugins can be unloaded, it's best to keep, for each plugin, a plain
old JS array object of the ids that have been passed to it, and root
that for the lifetime of the plugin.
If you really want to leak these jsids for the lifetime of the browser,
then bent's technique is the right thing (modulo the bug cdleary is
working on).
I am a little nervous that your bug might be something else entirely,
but we won't know until we get this much fixed. :-\
-j
Benjamin Smedberg
> On 5/2/11 2:13 PM, Benjamin Smedberg wrote:
>> But when jsids are passed in through hooks such as JSClass.getProperty,
>> the resulting NPIdentifier is only valid in the current call.
>
> There are at least three issues here:
> - jsid GC-safety
> - jsid hashability and equality
> - bugs
>
> Chris Leary is working the bugs; hashability and equality can wait a
> day or two as long as you don't forget about it. That leaves GC-safety.
>
> If plugins can be unloaded, it's best to keep, for each plugin, a
> plain old JS array object of the ids that have been passed to it, and
> root that for the lifetime of the plugin.
1) identifiers that the plugin explicitly asked for (via
NPN_GetStringIdentifier): these have to be rooted. We currently use the
interned-string technique, and it appears to work correctly.
2) identifiers that the plugin got in the course of answering
getproperty hooks. I think we don't want to intern these.
So in this particular cache which is currently incorrect, I want to keep
the permanent identifiers, and either
A. don't cache the non-interned ones
B. have some sort of GC hook to clear out the cache of the non-interned
values
Option B sounds over-complicated, so I'd really like to try option A.
(And assume that the current identifier-interning code is correct,
because it really does appear to be.)
--BDS
Chris Leary
> On 5/3/2011 10:37 AM, Jason Orendorff wrote:
>> On 5/2/11 2:13 PM, Benjamin Smedberg wrote:
> 1) identifiers that the plugin explicitly asked for (via
> NPN_GetStringIdentifier): these have to be rooted. We currently use the
> interned-string technique, and it appears to work correctly.
Are plugins often *un*loaded? If so, we can probably do much better by
providing the API users with an "interned jsid pool" that guarantees
permanence and O(1) comparisons for the lifetime of the pool, which can
autorelease at plugin unload time. That sounds like something a loving,
caring engine API would be happy to provide. <3
As jorendorff says, interning via the JSAPI keeps interned strings in
the runtime state until the browser shuts down, so bad plugin behavior
(like requesting a ton of these string identifiers) could cripple the
browser's memory usage even if the plugin were subsequently killed for
misbehaving.
> 2) identifiers that the plugin got in the course of answering
> getproperty hooks. I think we don't want to intern these.
If they get returned as NPIdentifiers that are assumed permanent we have
to root them. Interning strings via the API is a super-rooting,
sledgehammer-like mechanism.
> A. don't cache the non-interned ones
I'm a little confused here -- is it part of the plugin contract that the
getProperty results you mentioned are immediately dropped? Even if
that's the case, if the plugin does something that causes GC these jsids
have to be rooted in some way.
> B. have some sort of GC hook to clear out the cache of the non-interned
> values
I'm confused as to what's being cached, exactly, but if the jsids escape
into arbitrary plugin heap space, they must be appropriately rooted.
Thanks for the clarifications!
- Leary
Benjamin Smedberg
> On 05/03/2011 07:48 AM, Benjamin Smedberg wrote:
>> On 5/3/2011 10:37 AM, Jason Orendorff wrote:
>>> On 5/2/11 2:13 PM, Benjamin Smedberg wrote:
>> 1) identifiers that the plugin explicitly asked for (via
>> NPN_GetStringIdentifier): these have to be rooted. We currently use the
>> interned-string technique, and it appears to work correctly.
>
> Are plugins often *un*loaded? If so, we can probably do much better by
> providing the API users with an "interned jsid pool" that guarantees
> permanence and O(1) comparisons for the lifetime of the pool, which
> can autorelease at plugin unload time. That sounds like something a
> loving, caring engine API would be happy to provide. <3
not unloaded. They currently aren't because of bugs in the plugins.
Maybe in the future we will, but I don't think it affects anything else
we do here. We could certainly use a single pool that lasts the lifetime
of the app, since that's basically how the code is currently structured.
>
> As jorendorff says, interning via the JSAPI keeps interned strings in
> the runtime state until the browser shuts down, so bad plugin behavior
> (like requesting a ton of these string identifiers) could cripple the
> browser's memory usage even if the plugin were subsequently killed for
> misbehaving.
>
>> 2) identifiers that the plugin got in the course of answering
>> getproperty hooks. I think we don't want to intern these.
>
> If they get returned as NPIdentifiers that are assumed permanent we
> have to root them. Interning strings via the API is a super-rooting,
> sledgehammer-like mechanism.
The NPRuntime spec doesn't actually say how these should behave. I made
an incorrect assumption that all NPIdentifiers were permanent when I
implemented the cache in question. Our actual current behavior is that
they are not permanent identifiers: they are guaranteed to be valid for
the lifetime of the current stack frame. And since our implementation is
the de-facto standard, for the most part, I think we can make the spec
match our current behavior. So the only problem I have now is how to
make the cache behave in a sane way for non-permanent NPIdentifiers.
>> A. don't cache the non-interned ones
>
> I'm a little confused here -- is it part of the plugin contract that
> the getProperty results you mentioned are immediately dropped? Even if
> that's the case, if the plugin does something that causes GC these
> jsids have to be rooted in some way.
I'm not sure I understand the question about the "getProperty results",
but it's probably unrelated to the cache mentioned below. There may also
be a rooting bug for NPIdentifiers, but that looks pretty easy to solve,
at least if there is a public API in which this function can GC-trace
its identifier:
http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsJSNPRuntime.cpp?force=1#2292
>
>> B. have some sort of GC hook to clear out the cache of the non-interned
>> values
>
> I'm confused as to what's being cached, exactly, but if the jsids
> escape into arbitrary plugin heap space, they must be appropriately
> rooted.
The cache is an implementation detail of the multi-process plugin
implementation. When we remote NPRuntime calls across the plugin
boundary, we create a unique actor for each NPIdentifier we see in this
function:
http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginModuleParent.cpp#598
This cache was written under the assumption that all NPIdentifiers were
permanent, and it just permanently maps
NPIdentifier->PPluginIdentifierParent. Now that I know that some
NPIdentifiers are not permanent, I need to be able to know whether the
NPIdentifier I have right now is the permanent or non-permanent kind,
and do something differently in this function. I'm not sure (yet) what
I'm going to do differently: it may be that I'll just send over the
identifier value (int or string) instead of its identity, to avoid
lifetime issues. That will be slower in some cases, but probably not
noticeably.
--BDS
Christopher D. Leary
> We could certainly use a single pool that lasts the lifetime
> of the app, since that's basically how the code is currently structured.
The interning API is tied to the lifetime of the JSRuntime -- I think
that's the same concept as "the lifetime of the app" that you use here.
> Let's assume that plugins are reasonably well-behaved; they appear to be.
Okay, I was just hoping we could get some easy reliability wins from
OOPP and releasing interned strings when plugins go bad.
> Our actual current behavior is that
> they are not permanent identifiers: they are guaranteed to be valid for
> the lifetime of the current stack frame.
Great, so if I understand this correctly the NPIdentifiers must reside
on the stack as long as they are live, in which case the conservative
stack scanner will find and root them. If the contract says they *could*
escape into heap space under the plugin's control, then we would need to
root them before calling into the plugin code.
I just want to make sure this would be clear to plugin writers -- if you
write a function that places an NPIdentifier in the heap, then retrieves
it later from the heap in the same stack frame, and you've made
intervening calls that can trigger GC, you have a GC hazard.
> And since our implementation is
> the de-facto standard, for the most part, I think we can make the spec
> match our current behavior. So the only problem I have now is how to
> make the cache behave in a sane way for non-permanent NPIdentifiers.
That sounds good. The JSAPI function to ask if a string is interned will
soon represent its permanence.
> I'm not sure I understand the question about the "getProperty results",
> but it's probably unrelated to the cache mentioned below. There may also
> be a rooting bug for NPIdentifiers, but that looks pretty easy to solve,
> at least if there is a public API in which this function can GC-trace
> its identifier:
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsJSNPRuntime.cpp?force=1#2292
Sure can! JS_CALL_STRING_TRACER when the jsid is backed by a string. We
should also probably provide a JS_CALL_JSID_TRACER in the API to avoid
having the user figure out what JSID type it is.
> The cache is an implementation detail of the multi-process plugin
> implementation. When we remote NPRuntime calls across the plugin
> boundary, we create a unique actor for each NPIdentifier we see in this
> function:
>
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginModuleParent.cpp#598
Thanks for clarifying!
> I'm not sure (yet) what
> I'm going to do differently: it may be that I'll just send over the
> identifier value (int or string) instead of its identity, to avoid
> lifetime issues. That will be slower in some cases, but probably not
> noticeably.
Just to provide the option, we have a JS_SetGCCallback API function. If
you provide JSGC_MARK_END you can check JS_IsAboutToBeFinalized for the
backing strings in the cache and remove the corresponding
to-be-finalized entries.
The API doesn't offer any closure-like parameters, so you can only do
this in a static fashion. :-/ Not sure if that would work for you.
- Leary
Benjamin Smedberg
>
> That sounds good. The JSAPI function to ask if a string is interned
> will soon represent its permanence.
bool IsJSIDPermanent(jsid d) {
if (!JSID_IS_STRING(d))
return true;
return JS_StringHasBeenInterned(JSID_TO_STRING(d));
}
This assumes that IDs are only integers or strings, which is what
NPRuntime has: in jsapi.h it seems that there are also void and object
JSIDs, which has me vaguely worried. Do I need to worry about those
cases anywhere?
> Just to provide the option, we have a JS_SetGCCallback API function.
> If you provide JSGC_MARK_END you can check JS_IsAboutToBeFinalized for
> the backing strings in the cache and remove the corresponding
> to-be-finalized entries.
>
> The API doesn't offer any closure-like parameters, so you can only do
> this in a static fashion. :-/ Not sure if that would work for you.
It would be extremely complicated, so I'm going to avoid it if possible.
--BDS
Mook
enough to have no external consumers)
On 5/5/2011 11:55 AM, Christopher D. Leary wrote:
> On 05/05/2011 06:21 AM, Benjamin Smedberg wrote:
>> Our actual current behavior is that
>> they are not permanent identifiers: they are guaranteed to be valid for
>> the lifetime of the current stack frame.
>
> Great, so if I understand this correctly the NPIdentifiers must reside
> on the stack as long as they are live, in which case the conservative
> stack scanner will find and root them. If the contract says they *could*
> escape into heap space under the plugin's control, then we would need to
> root them before calling into the plugin code.
>
> I just want to make sure this would be clear to plugin writers -- if you
> write a function that places an NPIdentifier in the heap, then retrieves
> it later from the heap in the same stack frame, and you've made
> intervening calls that can trigger GC, you have a GC hazard.
Nope, this isn't clear at all, in particular due to
NPClass.NPSetPropertyFunctionPtr, where the documentation appears to be
"this function pointer exists". Of course, from a cursory search via
Google code, it appears that most implementations simply raise an
exception there :)
--
Mook