In the NPAPI bridge to the JS engine we have a bug 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
> <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?
An alternate suggestion on IRC was to skip this cache step for 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.
> 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. :-\
> 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.
Hrm, I think there are two different kinds of identifiers here:
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.)
> 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.
> 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
For the purposes of this discussion, we should assume that plugins are 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.
Let's assume that plugins are reasonably well-behaved; they appear to be.
>> 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/nsJSNP...
>> 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:
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.
> 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/nsJSNP...
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:
> 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.
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.
(I occasionally play NPAPI developer, though I'm new at it and am lucky 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 :)
