The patch in bug 787927 prevents self-hosted scripts from showing up in the debugger and most places where stack introspection is done in the browser. Unfortunately, it left a few holes, with most of them in the old debug API. This patch plugs those holes. Additionally, it adds (de-)serialization of the `selfHosted` flag to js::XDRScript. I doubt that self-hosted scripts will ever be XDRed, but wouldn't entirely bet on it. With the introduction of a `selfHosted` flag on JSScript, the equivalent flag in JSFunction should be removed. I'll do that in a follow-up bug, however, as I want this one to be uplifted to Aurora.

Comment on attachment 727652 [details] [diff] [review] v1 Review of attachment 727652 [details] [diff] [review]: ----------------------------------------------------------------- Just two notes; one we discussed on IRC, the other possibly new. ::: js/src/jsdbgapi.cpp @@ +73,5 @@ > > static bool > IsTopFrameConstructing(JSContext *cx, AbstractFramePtr frame) > { > + NonBuiltinScriptFrameIter iter(cx); I wonder if this is correct. It doesn't seem to me that we take any steps to prevent self-hosted frames from reaching cx->runtime->debugHooks->executeHook and ...->callHook. If we don't take such steps, but we use NonBuiltinScriptFrameIter here in IsTopFrameConstructing, then I would expect executeHook and callHook to be passed 'isConstructing' values that don't match the 'frame' they're passed. ::: js/src/jsscript.cpp @@ +405,5 @@ > IsGenerator, > IsGeneratorExp, > OwnSource, > + ExplicitUseStrict, > + SelfHosted Whenever you change the format of XDR'd scripts, be sure to bump the value of XDR_BYTECODE_VERSION in js/src/vm/Xdr.h, according to the comments there. (Unless this is a backwards-compatible change, which it might be...)

Thanks for the clarification on IRC! Turns out I don't need to guard in js::ScriptDebugEpilogue: frame.maybeHookData() will always return NULL there. Oh and no: the XDR format change isn't compatible, thanks.

Without a more explicit user impact, we can't track for release. Please re-nominate if we know of an important user scenario affected by this bug.

Comment on attachment 727978 [details] [diff] [review] v2 Review of attachment 727978 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me!

Thanks for the review! https://hg.mozilla.org/integration/mozilla-inbound/rev/495bf367cca3

(In reply to Till Schneidereit [:till] from comment #2) > Turns out I don't need to guard in js::ScriptDebugEpilogue: > frame.maybeHookData() will always return NULL there. Is this because we know the ScriptDebugPrologue call didn't set any hook data, because we did guard there? That sounds correct, but it sure would be nice to have an assertion and a comment in ScriptDebugEpilogue.

(In reply to Jim Blandy :jimb from comment #6) > (In reply to Till Schneidereit [:till] from comment #2) > > Turns out I don't need to guard in js::ScriptDebugEpilogue: > > frame.maybeHookData() will always return NULL there. > > Is this because we know the ScriptDebugPrologue call didn't set any hook > data, because we did guard there? > > That sounds correct, but it sure would be nice to have an assertion and a > comment in ScriptDebugEpilogue. Yes, that's the reasoning. You're right, though: a comment about that seems in order. Added in https://hg.mozilla.org/integration/mozilla-inbound/rev/0c2c54c2b5da

Comment on attachment 727978 [details] [diff] [review] v2 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 784294, bug 787927 User impact if declined: Some addons (especially GreaseMonkey) fail, plus some top-crashers caused by the attempted fix in bug 787927 (see bug 851788) Testing completed (on m-c, etc.): yes, patch has landed 1.5 weeks ago Risk to taking this patch (and alternatives if risky): slim to none. String or IDL/UUID changes made by this patch: none I dropped the ball on requesting Aurora approval before the uplift, but this should definitely go into beta, now. Sorry.

Comment on attachment 727978 [details] [diff] [review] v2 Approving the low risk fix to help fix a Fx21 top crasher 851788.

Comment on attachment 727978 [details] [diff] [review] v2 https://hg.mozilla.org/releases/mozilla-beta/rev/dfd9d55369da