832435 - Compartment mismatch when using javascript: urls in locationbar

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

https://crash-stats.mozilla.com/report/index/bp-d8b6da1e-7be4-4886-99c2-0cbba2130118

Seems to be regression from bug 824864.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Do we really not have tests for this?

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Fix compartment handling for EvaluteString and javascript: uris. v1

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 4

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 5

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbbb6e3d240

Comment 6

[Phil Ringnalda (:philor)]

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."

Comment 7

[Bobby Holley (:bholley)]

Attachment: Fix compartment handling for EvaluteString and javascript: uris. v2 r=bz

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

Comment 8

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=e2a4897eaf2c

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Bobby Holley (:bholley)]

(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

Comment 11

[Jonathan Kew [:jfkthame]]

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

Comment 12

[Bobby Holley (:bholley)]

(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?

Comment 13

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/332f32b1fad9

Comment 14

[Boris Zbarsky [:bzbarsky]]

> There's a comment in the code there referring to exiting the JSAutoCompartment

Oh, right.  Nevermind me!

Comment 15

[Justin Dolske [:Dolske]]

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.

Comment 16

[Phil Ringnalda (:philor)]

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?

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

Comment 18

[Bobby Holley (:bholley)]

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.