647727 - setTimeout doesn't work after calling alert from evalInSandbox

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Crossrider]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-US) AppleWebKit/534.16 (KHTML, like Gecko) Chrome/10.0.648.204 Safari/534.16
Build Identifier: Firefox 4.0

We have a simple evalInSandbox code with alert and then immediately the setTimeout (or setInterval) function, which has a simple alert inside it too.

The first alert executes, but then the alert in the setTimeout itself does not.
If we run the setTimeout on its own without the alert it works:

WORKING CODE EXAMPLE:
----
setTimeout(function() { alert('timeout') },2000);
----

NOT WORKING EXAMPLE:
----
alert("start");
setTimeout(function() { alert('timeout') },2000);
-----

After it stops working, even if you run the code without the alert, the setTimeout alert will not run.
ONLY restarting Firefox make it work again, but only when there is no alert prior to the seTimeout (or setInterval)

Reproducible: Always

Steps to Reproduce:
1.create an xpi with the which runs the above code with evalInSandbox.
2.see that it does work without alert
3.add the alert -- see that timeout is not executing
4. try again without alert - this time the timeout its not working
5. restart firefox
6. see how it works again, but not with the alert prior to the setTimeout.

repeat.
Actual Results:  
setTimetout and setInterval not working after alert in evalInSandbox

Expected Results:  
setTimeout and setInterval should work even after alert in evalInSandbox

the new alert design is not cool and sometime simply hangs.. but that's a whole different issue..

Comment 1

[Anthony Lieuallen]

Attachment: reproducible test case for alert-in-sandbox breaking settimeout

This bug seriously affects Greasemonkey.  It will also break code _in the page_.

See our reports:
https://github.com/greasemonkey/greasemonkey/issues/1318
http://groups.google.com/group/greasemonkey-users/t/aed1ecc36fa9e404

Real users are having real problems because of this.  I came up with a very similar test case as a greasemonkey script (which will execute inside a sandbox):
https://gist.github.com/910628


In addition, this attachment is a simple test case which just calls alert, and a setTimeout for a second alert, from within a sandbox.  The second never shows.  It works fine in Firefox 3.6, but fails in 4.0.

Here's the actual JS code involved:
---------------------------------
function alertSettimeoutExercise() {
  var contentWin = gBrowser.contentWindow;
  var sandbox = new Components.utils.Sandbox(contentWin);
  sandbox.__proto__ = contentWin;
  Components.utils.evalInSandbox(
      'alert("first alert; works");' +
      'setTimeout(function() { alert("second alert; fails"); }, 0)',
      sandbox);
}
---------------------------------
In case it's easier for you to see it here than to dissect the XPI I attached.


Moreover, try this:

Open any page.  Paste (e.g.) into the address bar:

javascript:setTimeout(function() { alert("alert via timeout"); }, 0);void(0)

And you'll see an alert.  Trigger the exercise of this bug and paste it again: it will not work.  Content setTimeout() calls are broken after the call to alert() inside a sandbox referencing that content.  This breakage of content lasts for the entire life of the tab; navigating to a new page and pasting that snippet will still fail to show the expected alert, once the alert-in-sandbox has broken it.

I don't know the right flags to set to mark this as high priority (and won't risk doing the wrong thing), but please consider it as such.

Comment 2

[Piyush Soni]

My god. This is a critical bug, worth releasing a FF Patch today!

Comment 3

[David Dahl :ddahl]

I tested this in the web console - whcih executes everything in a sandbox:

alert("start");
setTimeout(function() { alert('timeout') },2000);

Comment 4

[David Dahl :ddahl]

(In reply to comment #3)
> I tested this in the web console - whcih executes everything in a sandbox:
> 
> alert("start");
> setTimeout(function() { alert('timeout') },2000);

I also meant to write that it does indeed seem to be a problem

Comment 5

[Boris Zbarsky [:bzbarsky]]

I can't see how this is possibly a JSeng issue.

Chances are, it's an issue with the Firefox-side alert code (which suppresses alerts while other alerts are live).

Justin, can you take a look?

Comment 6

[Justin Dolske [:Dolske]]

Hmm, strange.

Testing from the web console, I ran do |setTimeout(function() { alert('timeout') },2000);| repeatedly, and it works each time. But as soon as I do just a plain alert() without the timeout, doing calls with the setTimeout do nothing. I don't even hit a breakpoint at the top of nsGlobalWindow::Alert().

Next would be to see if the timeout's actually firing at all. Oh, hmm, I also see some errors in the console saying the NS_ENSURE_ARG_POINTER failed in nsDOMWindowUtils::LeaveModalStateWithWindow(). That's probably causing the timers to not fire.

Will look more tomorrow.

Comment 7

[Justin Dolske [:Dolske]]

Ah. So here's what's _happening_.

When the first prompt is shown, EnterModalState can't figure out the calling window...

6373   JSContext *cx = nsContentUtils::GetCurrentJSContext();
6374 
6375   nsCOMPtr<nsIDOMWindow> callerWin;
6376   nsIScriptContext *scx;
6377   if (cx && (scx = GetScriptContextFromJSContext(cx))) {
6378     scx->EnterModalState();
6379     callerWin = do_QueryInterface(nsJSUtils::GetDynamicScriptGlobal(cx));
6380   }

Specifically, scx is NULL, and so EnterModalState returns a null DOM window.

When the prompt is dismissed, we call...

1492 nsDOMWindowUtils::LeaveModalStateWithWindow(nsIDOMWindow *aWindow)
1493 {
1494   NS_ENSURE_ARG_POINTER(aWindow);
1495   mWindow->LeaveModalState(aWindow);
1496   return NS_OK;
1497 }

aWindow is NULL here, and so we never actually leave the modal state. Thus timeouts are suppressed, and the second prompt is never shown because the timer isn't firing (presumably, I didn't actually verify that).

Relaxing the NS_ENSURE_ARG_POINTER to NS_WARN_IF_FALSE seems to fix things, but II don't think that's actually right. We had to add the "...WithWindow" flavors of these calls to make sure we enter and leave modal state on the right thing, which is slightly tricky because it's called from script (nsPrompter.js). If we're getting a NULL window here, there could be problems with a window improperly receiving events, or with the slow script watchdog getting confused.

Ccing mrbkap for the moment.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 6373   JSContext *cx = nsContentUtils::GetCurrentJSContext();

So this is coming back with the sandbox context, right?

The problem is that in this case the script doing the alert is really not running inside a window at all.  It seems to me that in cases like that we should simply use |this| as the window in EnterModalState (and use mContext as scx).

Comment 9

[Anthony Lieuallen]

Do we have a timeline for this being fixed?  Any chance for Firefox 5?

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

So, I think the fix to this bug is pretty easy, but a little fragile. I agree with bz about using this as the window and mContext as scx. Dolske, we don't actually have to worry about the slow script dialog because, right now, one can't pop up on the sandbox context (there's no context private, meaning that we take the first early return in nsJSContext::DOMOperationCallback).

So, the easiest way to fix this bug would be to use LeaveModalState if EnterModalStateWithWindow returns null.

The only thing I don't really like about that setup is that it hinges on the early return in DOMOperationCallback, but I don't see a really clean way of modeling that relationship.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Add comments in the two places pointing at each other?

Comment 12

[Anthony Lieuallen]

Any progress here?  Greasemonkey is still suffering this bug, with an ugly workaround in place.  We've now seen the release of a "major" version (does this make sense any more?) without a fix.

Comment 13

[Anthony Lieuallen]

Still wondering when this might be fixed.  Firefox 6?

Comment 14

[Piyush Soni]

Anyone fixing this? Firefox 7?

Comment 16

[David Chan [:dchan]]

From bug# 678971, this bug also breaks certain websites if a tab modal function is called from scratchpad/webconsole.

Comment 17

[:aceman]

I see this on Firefox 8a2 on my page with simple JS. I am not aware of any evalInSandbox, I think I am not using it.

Comment 18

[Josh Matthews [:jdm]]

aceman, could you file that a new bug and provide a testcase or URL that demonstrates that behaviour, please? It doesn't necessarily sound like the same bug as this one.

Comment 19

[Boris Zbarsky [:bzbarsky]]

And please mention the new bug's number here?

Comment 20

[:aceman]

As described in the original report, FF must get into a state when it stops working. Then after a restart it works again. As always, I am now unable to reproduce it ;) If I come across the state again I will surely try to make a testcase.

Comment 21

[pqwoerituytrueiwoq]

Calling the prompt function also does this
Mozilla/5.0 (X11; Linux x86_64; rv:6.0.2) Gecko/20100101 Firefox/6.0.2 ID:20110906071345

Comment 22

[pqwoerituytrueiwoq]

(In reply to pqwoerituytrueiwoq from comment #21)
> Calling the prompt function also does this
> Mozilla/5.0 (X11; Linux x86_64; rv:6.0.2) Gecko/20100101 Firefox/6.0.2
> ID:20110906071345

forgot to mention this bug appears to be tab specific once it is triggered it will affect that tab till it is closed or firefox is restarted

Comment 24

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

Bug 693197 is the same issue except with the developer tools instead of evalInSandbox.

Comment 25

[Anthony Lieuallen]

Any progress on this bug?  Greasemonkey still has an awful alert-only workaround in place.  And users are still hitting and reporting this bug for its other slightly-more-rare variants (prompt):

https://github.com/greasemonkey/greasemonkey/issues/1483

Comment 26

[Alice0775 White]

Workaround:
set prompts.tab_modal.enabled to false

Comment 27

[Anthony Lieuallen]

Any progress at on this bug?

Comment 28

[Tim Martin]

This bug still seems to be an issue -- just saw someone cussing out Firefox for this in the online forum of a web game whose users frequently employ GM scripts.  :)

Comment 29

[Anthony Lieuallen]

We're rapidly coming up on this bug being open for a year.  Will we/when will we see this fixed?

Comment 30

[Piyush Soni]

Please fix this! It just affected my script as well. :(

When it gives one alert, my settimeout which is supposed to work every 900 milliseconds in a specific condition randomly fails, silently! :( So bad !! And people curse us script developers. 

I closed and reopened tabs, and the only thing that worked was a browser restart! 

Can someone tell what are the technical difficulties in fixing this?

Comment 31

[Boris Zbarsky [:bzbarsky]]

I just tried the attached XPI in a trunk nightly, and I see two alerts.  

I also don't see the problem in the Web Console, using the steps from comment 3.

So as far as I can tell, this bug is fixed.  I'll take a quick look into what might have fixed it.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Fix range seems to be http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5891cc95eac7&tochange=95d1bb200f4e

In that range, bug 754202 seems most likely to have fixed this.  Bobby, does that seem plausible?

Comment 33

[Boris Zbarsky [:bzbarsky]]

This is weird.  We're _still_ pushing the sandbox context (which has a null nsIcriptContext) on the JSContext stack.  So the issue comment 7 describes seems like it should still happen...

Bobby, is something pushing some other JSContext on the stack somewhere here?  Did you change the behavior of nsContentUtils::GetCurrentJSContext?

Comment 34

[Boris Zbarsky [:bzbarsky]]

Ah, looks like the key difference is that LeaveModalStateWithWindow got changed in bug 741587 to not throw on null.

So this is fixed, a far as I can tell.

Comment 35

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #32)
> In that range, bug 754202 seems most likely to have fixed this.  Bobby, does
> that seem plausible?

Those patches were backed out...

Comment 36

[Boris Zbarsky [:bzbarsky]]

> Those patches were backed out...

See comment 34.

Comment 37

[Anthony Lieuallen]

Downloaded nightly.  Confirmed that attached test case works as expected.

Installed patched greasemonkey (without our workaround for this bug).  Put in a user script the "not working example" from the original comment.  It works.

Confirmed fixed.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Anthony, thanks for confirming that!