It's on the chopping block because its semantics are bogus (or so I'm told). This call was added in bug 810082 (as js::IsContextRunningJS(cx)). If anyone involved there could explain what it's supposed to mean, that'd be great.

The reason is because plugins are awful and expect to be fully transparent to JS -- if you create an object, set its type to flash, then call <object>.someFlashFunction() it is expected to immediately succeed. So, when this call asks for the plugin instance, we take special care to spawn it *right then* if not yet done. However, this is a huge jank, as plugins suck and spawn slowly. So, the callerIsContentJS check allows this behavior to only be invoked when we're access the prototype as part of a content JS call, and be non-complaint at other times. When that call was added it was the result of talking to several XPC/JS people, so I'm not sure what the replacement is or what purpose it serves in that conditional.

Ideally, the triggered-by-content and triggered-by-native-code cases would have orthogonal API entry points. Could you elaborate on the set of situations comprising each case? If we can't fix it that way, replacing this with !!nsContentUtils::GetCurrentJSContext() is a better way to check whether the JS engine is active at this time.

(Also, you should take the :johns out of your Ancient Account name, because it makes it an ambiguous match :P )

(In reply to Bobby Holley (:bholley) from comment #2) > Ideally, the triggered-by-content and triggered-by-native-code cases would > have orthogonal API entry points. Could you elaborate on the set of > situations comprising each case? > > If we can't fix it that way, replacing this with > !!nsContentUtils::GetCurrentJSContext() is a better way to check whether the > JS engine is active at this time. The cases are roughly: [var obj = the <object>] A) Random chrome click-to-play code OnPluginConfigured(obj) { pluginObjects.append( obj ); /* Do things with obj that run serious risk of requiring its prototype */ } B) Random webpage code obj.Hi(); ... Whenever the JS prototype is built (what used to be called DoNewResolve but I don't know the current state of things) we used to ensure the plugin was spawned if applicable so it could expose its functions on the prototype chain. This was janky, since everything touches the object all the time in chrome, essentially forcing a blocking flash-spawn nearly immediately after the <object> was created/configured. Avoiding this happening accidentally was whack-a-mole, since the chrome click-to-play code really needs to know about/touch the <object> in JS. The fix was: DoNewResolve() { if ( !isChromeDoingThis ) { SpawnPluginNow() } } And evolved into the current thing. I certainly don't know enough about JS object lifetimes to say if there's a better way to do this -- the current code was the result asking someone (It might've been you!) how best to accomplish this.

Ok. If comment 4 is the motivation, I think we can just remove the JS_IsRunning call. Checking that aCx is non-null a and that the subject principal is non-system should be enough.

You want to do that or should I?

(In reply to :Ms2ger from comment #6) > You want to do that or should I? It's a one-line patch, but I'm guessing that johns doesn't have a recent checkout of the code, so it would be nice if you could do it. :-)

Comment on attachment 8676830 [details] [diff] [review] Remove the JS_IsRunning call in nsObjectLoadingContent::ScriptRequestPluginInstance Review of attachment 8676830 [details] [diff] [review]: ----------------------------------------------------------------- For reference, it looks like this was added in bug 810082.

ms2ger++