Last Comment Bug 647727 - setTimeout doesn't work after calling alert from evalInSandbox
: setTimeout doesn't work after calling alert from evalInSandbox
Status: RESOLVED FIXED
: regression
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: 2.0 Branch
: All All
: -- critical with 12 votes (vote)
: mozilla16
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
: 693197 (view as bug list)
Depends on: 741587
Blocks: 59314 621764
  Show dependency treegraph
 
Reported: 2011-04-04 13:05 PDT by Crossrider
Modified: 2012-06-27 12:50 PDT (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
reproducible test case for alert-in-sandbox breaking settimeout (1.62 KB, application/x-xpinstall)
2011-04-12 07:46 PDT, Anthony Lieuallen
no flags Details

Description Crossrider 2011-04-04 13:05:58 PDT
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 2011-04-12 07:46:27 PDT
Created attachment 525378 [details]
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 2011-04-14 09:15:49 PDT
My god. This is a critical bug, worth releasing a FF Patch today!
Comment 3 David Dahl :ddahl 2011-04-20 11:04:03 PDT
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 2011-04-20 11:23:28 PDT
(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 [:bz] (still a bit busy) 2011-04-20 18:31:27 PDT
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] 2011-04-20 19:05:53 PDT
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] 2011-04-21 17:23:31 PDT
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 [:bz] (still a bit busy) 2011-04-21 17:37:07 PDT
> 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 2011-05-26 08:29:54 PDT
Do we have a timeline for this being fixed?  Any chance for Firefox 5?
Comment 10 Blake Kaplan (:mrbkap) 2011-06-01 09:09:41 PDT
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 [:bz] (still a bit busy) 2011-06-01 09:21:01 PDT
Add comments in the two places pointing at each other?
Comment 12 Anthony Lieuallen 2011-06-23 08:58:57 PDT
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 2011-07-08 10:26:32 PDT
Still wondering when this might be fixed.  Firefox 6?
Comment 14 Piyush Soni 2011-08-30 14:44:42 PDT
Anyone fixing this? Firefox 7?
Comment 15 David Chan [:dchan] 2011-09-01 16:41:39 PDT
*** Bug 678971 has been marked as a duplicate of this bug. ***
Comment 16 David Chan [:dchan] 2011-09-01 16:47:56 PDT
From bug# 678971, this bug also breaks certain websites if a tab modal function is called from scratchpad/webconsole.
Comment 17 :aceman 2011-09-05 12:10:36 PDT
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] 2011-09-05 14:56:38 PDT
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 [:bz] (still a bit busy) 2011-09-05 18:24:28 PDT
And please mention the new bug's number here?
Comment 20 :aceman 2011-09-06 10:13:42 PDT
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 2011-09-26 09:00:24 PDT
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 2011-09-26 09:07:27 PDT
(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 23 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-09 15:36:47 PDT
*** Bug 693197 has been marked as a duplicate of this bug. ***
Comment 24 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-09 15:38:23 PDT
Bug 693197 is the same issue except with the developer tools instead of evalInSandbox.
Comment 25 Anthony Lieuallen 2011-12-08 08:40:49 PST
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 2011-12-08 08:49:58 PST
Workaround:
set prompts.tab_modal.enabled to false
Comment 27 Anthony Lieuallen 2012-02-16 14:05:06 PST
Any progress at on this bug?
Comment 28 Tim Martin 2012-03-05 08:14:06 PST
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 2012-03-19 07:22:42 PDT
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 2012-05-21 17:42:09 PDT
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 [:bz] (still a bit busy) 2012-06-26 10:02:30 PDT
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 [:bz] (still a bit busy) 2012-06-26 10:12:40 PDT
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 [:bz] (still a bit busy) 2012-06-26 12:36:18 PDT
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 [:bz] (still a bit busy) 2012-06-26 12:47:23 PDT
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) (busy with Stylo) 2012-06-26 13:05:43 PDT
(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 [:bz] (still a bit busy) 2012-06-26 13:09:00 PDT
> Those patches were backed out...

See comment 34.
Comment 37 Anthony Lieuallen 2012-06-27 12:41:16 PDT
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 [:bz] (still a bit busy) 2012-06-27 12:50:29 PDT
Anthony, thanks for confirming that!

Note You need to log in before you can comment on or make changes to this bug.