614151 - setTimeout(close) closes a window the page isn't allowed to close

Status: RESOLVED FIXED

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

Comment 1

[Jesse Ruderman]

Attachment: testcase (closes window)

Comment 2

[Jesse Ruderman]

If there aren't any other tabs in the window, I get this assertion failure:

###!!! ASSERTION: should be executing script: 'JS_IsRunning(mContext)', file dom/base/nsJSEnvironment.cpp, line 3551

Comment 3

[Jesse Ruderman]

This is sg:low on its face, but could turn out to be worse.

Comment 4

[Johnny Stenback (:jst)]

The reason for this is that we end up running the timeout w/o any script on the JS stack, only a native function call, so we think various capabilities are enabled, even though they are not.

Blake, seems like this could be bad if something other than close is passed to setTimeout(). Blocking for now.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

This was broken by the move to entirely-fast natives. We used to create stack frames for XPConnect functions, such as alert and close; however now we don't, and we fall into the "no script code running" case. This patch makes IsCapabilityEnabled respect the principals pushing stack and makes nsJSContext::CallEventHandler clamp the principal for this case exactly. I'll file a bug on fixing JS_IsRunning (which was also broken) and a second bug for making nsScriptSecurityManager get object principals from the compartment, which will be a lot faster than the current code.

Comment 7

[Johnny Stenback (:jst)]

Cc:ing reporter of duplicate bug so he can track when this gets resolved.

Comment 8

[Johnny Stenback (:jst)]

I pushed this to try along with another change that enabled a few tests that were disabled in the compartment landing, and it looks like this causes mochitest failures. The results are at:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=36d1ea34ff7b

The reftest failures are not caused by this, but I believe the mochitest ones are (1 and 4).

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better patch

So, there was a glaring bug in the old patch (it didn't always set result), but this patch still fails tinderbox even with that fixed. The problem is that we have a test (layout/forms/test/test_bug536567.html) that sets a JS object as a handler on a file chooser. So the C++ stack looks like
<event loop>
<file chooser stuff>
<xpconnect stuff>
<JS impl of file chooser>

the JS impl grabs UniversalXPConnect and attempts to return a XOW (cross origin file) back to C++. Because when we obtain the reference to the file object, we have UniversalXPConnect, we're allowed to manipulate it. However, when we return it, XPConnect has to unwrap it. And by that time, we've popped the JS off the stack, and lost the UniversalXPConnect privileges. So XPConnect balks trying to convert a XOW into a C++ object and things break.

I don't have any bright ideas about how to fix this, other than changing the test.

Comment 10

[Johnny Stenback (:jst)]

I think given what we're dealing with here, changing this one test is a fine option.

Comment 11

[(no longer active)]

Attachment: Part 2: convert the test for bug 536567 to a chrome test

jst asked me on IRC to convert the test for bug 536567 to a chrome test.  This is the simplest and quickest way to do so.  I've tested it to make sure that with these two patches, the test in question passes successfully.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 502914 [details] [diff] [review]
Part 2: convert the test for bug 536567 to a chrome test

I like it! Thanks Ehsan!

Comment 13

[(no longer active)]

I landed my part, since it's not really dependent on mrbkap's part:

http://hg.mozilla.org/mozilla-central/rev/775cfd3f2c87

Comment 14

[(no longer active)]

I backed it out because of an orange:

http://hg.mozilla.org/mozilla-central/rev/603f80cfc7fb
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1294789779.1294791160.21310.gz&fulltext=1

It seems like the subframe HTML file can't be found.  I'm doing a Linux build locally to investigate.

Comment 15

[(no longer active)]

Attachment: Part 2: convert the test for bug 536567 to a chrome test

The subframe file should be accessible through http, and therefore should not be packaged in the chrome package.  This should solve the failure (the reason I didn't see this failure locally was that I had that file in the correct path in my objdir).

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 503003 [details] [diff] [review]
Part 2: convert the test for bug 536567 to a chrome test

Thanks a lot for doing this, Ehsan!

Comment 17

[(no longer active)]

I pushed this to try just to check my sanity, and if the results are positive, I will push it to mozilla-central tomorrow.  Blake, if you think you'll need to land it sooner, you should keep an eye on the try results on this revision: 2b9fa7c33458

Comment 18

[(no longer active)]

Relanded the test patch:

http://hg.mozilla.org/mozilla-central/rev/aa76da2d163e

Comment 19

[Johnny Stenback (:jst)]

Fix landed:

http://hg.mozilla.org/mozilla-central/rev/910fb1e279d1

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 1.9.2 patch

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 537629 [details] [diff] [review]
1.9.2 patch

Approved for 1.9.2.18, a=dveditz for release-drivers

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 537629 [details] [diff] [review]
1.9.2 patch

Guess this didn't make 3.6.18 -- next time.

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Blake, can you land this on 1.9.2, please?

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6991b08bcf48

Comment 25

[Al Billings [:abillings - ex-MoCo]]

(In reply to Jesse Ruderman from comment #1)
> Created attachment 492563 [details]
> testcase (closes window)

This testcase doesn't do anything in 3.6.19 on OS X. If I run it in a window by itself or in a browser with multiple tabs, nothing happens. Was the problem behavior actually observed in 1.9.2?

Comment 26

[Al Billings [:abillings - ex-MoCo]]

https://bugzilla.mozilla.org/show_bug.cgi?id=650252#c6 references this bug. Are the exploits related as that one is definitely fixed and can be verified.