Last Comment Bug 771977 - unresponsive scripts in sandboxes associated with the hidden window are never terminated
: unresponsive scripts in sandboxes associated with the hidden window are never...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 756588
  Show dependency treegraph
 
Reported: 2012-07-08 21:57 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-07-17 02:12 PDT (History)
5 users (show)
gavin.sharp: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (1.03 KB, patch)
2012-07-08 22:51 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
jst: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-08 21:57:40 PDT
The FrameWorker component created in bug 762569 creates iframes in the hidden window, and uses them as contexts for sandboxes, which then evaluate scripts retrieved from the social providers.

It's possible for the social provider's script to have bugs that could trigger an infinite loop, or otherwise unresponsive script. Right now, when this happens the browser becomes entirely unresponsive with no recourse - the slow script dialog doesn't appear, presumably because it can't usefully parent itself to the hidden window.

We need some mechanism to ensure that these situations don't result in a totally hosed browser. One way to achieve this may be to just cancel the script automatically if the slow script dialog fails to display, or something like that.

Related note: it would be awesome if we could somehow detect that the script had been killed, so that we can disable the guilty provider and/or notify the user.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-08 21:59:58 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #0)
> One way to achieve this may be to just cancel the
> script automatically if the slow script dialog fails to display, or
> something like that.

Hmm, this already seems to be what nsJSContext::DOMOperationCallback does. I'll investigate why this isn't working.
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-08 22:43:25 PDT
Oh, I was wrong, the case being hit is this one:

  rv = prompt->ConfirmEx(title, msg, buttonFlags, waitButton, stopButton,
                         debugButton, neverShowDlg, &neverShowDlgChk,
                         &buttonPressed);
...
  if (NS_FAILED(rv) || (buttonPressed == 0)) {
    // Allow the script to continue running

NS_FAILED(rv) is true, so we let the script continue running.

Can we just change that? What cases would we fail to prompt and want to continue running unresponsive scripts? This behavior seems to go back all the way to bug 13350, and I don't know that it's ever mattered in practice before now.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-08 22:51:56 PDT
Created attachment 640143 [details] [diff] [review]
proposed patch

This is the minimal workable solution - terminates the runaway script after the normal prompt timeout.

Ideally, we'd limit these scripts even further than that, and have some way to detect this occurring.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2012-07-10 18:58:12 PDT
Comment on attachment 640143 [details] [diff] [review]
proposed patch

I'm generally fine with this change, it means when unexpected things fails, we terminate scripts, as opposed to what we do now which is to let them run. I don't think we ever had a case where we needed this to work one way or another, it just seemed to be the safer bet, but in this case it's not. Alternatively we could of course write code that would explicitly check if we're running in a sandbox and make a decision based on that, but I don't think that's worth the effort right now.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-15 23:18:17 PDT
https://tbpl.mozilla.org/?tree=Try&rev=3124715357b3
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-16 12:20:47 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/75b6ec6db468
Comment 7 Ed Morley [:emorley] 2012-07-17 02:12:50 PDT
https://hg.mozilla.org/mozilla-central/rev/75b6ec6db468

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