https://crash-stats.mozilla.com/report/index/bp-d8b6da1e-7be4-4886-99c2-0cbba2130118 Seems to be regression from bug 824864.

Do we really not have tests for this?

The basic issue here is that the return value of EvaluateString is in an arbitrary context, and we can't wrap it into a known context because we don't know what cx the caller is using. I was hoping to just hoist EvaluateString into nsJSUtils and make the cx argument explicit. But it's not clear to me exactly how to port over the TerminationFuncHolder stuff, and I don't want to do anything dangerous with that right now. So I'm going to just wrap the value into initial compartment of mContext, and make callers either use that context or explicitly wrap. Hopefully we can clear this stuff up better soon.

Turns out there was also an issue in the sandbox path for JS URIs. Boris, if this all looks ok, feel free to push it to inbound so that we can get this on Nightly ASAP.

Comment on attachment 704149 [details] [diff] [review] Fix compartment handling for EvaluteString and javascript: uris. v1 r=me

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f02b8f754b74 - everything failed when it hit "Assertion failure: cx->runtime->requestDepth || cx->runtime->isHeapBusy(), at ../../../js/src/jscntxt.cpp:1513."

Oh jeeze. I could have sworn I ran some smoke mochitest-plain tests on this, but apparently not. Carrying over review.

That violates the "exit the request before we pop the cx" comment, no?

(In reply to Boris Zbarsky (:bz) from comment #9) > That violates the "exit the request before we pop the cx" comment, no? I'm not sure what comment you're referring to. There's a comment in the code there referring to exiting the JSAutoCompartment, which we need to do before popping the cx. But I don't see anything relating to the JSAutoRequest, and it doesn't seem to me like it's a problem. We keep a JSAutoRequest alive across push/pop in a number of other places in nsJSEnvironment.cpp. I'm concerned that I'm missing something important here. But since this is green on try and fixes a number of regressions and crashes, and since you're probably out today, I'm going to optimistically land it with your carried-over review. I apologize if I'm missing a legitimate problem here, and will address it immediately in a followup patch. :-) https://hg.mozilla.org/integration/mozilla-inbound/rev/332f32b1fad9

The test added here has failed at least once already on inbound; is it destined to be a new intermittent orange? :( https://tbpl.mozilla.org/php/getParsedLog.php?id=18992388&tree=Mozilla-Inbound

(In reply to Jonathan Kew (:jfkthame) from comment #11) > The test added here has failed at least once already on inbound; is it > destined to be a new intermittent orange? :( > > https://tbpl.mozilla.org/php/getParsedLog.php?id=18992388&tree=Mozilla- > Inbound I sure hope not. Luckily the test is only a dozen lines and should be quick to grok for anyone who knows browser-chrome stuff better than I do. Dolske, do I need to do some waitForFocus or something in here?

> There's a comment in the code there referring to exiting the JSAutoCompartment Oh, right. Nevermind me!

Uhm. browser_bug832435.js | uncaught exception - uncaught exception: SyntaxError: missing ; before statement at :0 While our line reporting is a little unfortunate (especially when it's for a javascript URI), this doesn't seem too mysterious... gURLBar.inputField.value = "javascript: document.body.innerHTML = '11111111'); "; What you doing there, close-paren? Go home. You are drunk.

Yup, every single time that URI gets loaded, it will produce a SyntaxError. Remove the paren, and every single time it will produce a ReferenceError from document being undefined. We run browser-chrome around a dozen times per push, and we've probably run on a hundred or so pushes since it landed, and it has failed exactly once. Does that mean it's okay to hit a syntax error in a browser-chrome test as long as you call finish() before it can get reported, or does it mean that out of 1200 runs, the test has only actually loaded the URI once?

The latter, I bet. javascript: URIs run async. I should clearly have read the test more carefully.... :(

Good call. The tests reliably crashed without the patch, but that's probably because the compartment mismatch occurred no matter what, even if the page was too long-gone at that point to notice any errors thrown from the script. Ideally the javascript: URI would dispatch an event or something, but the whole point here is that these things get evaluated in a sandbox where they have no access to anything useful. But a SimpleTest.executeSoon probably does the job here.