1282671 - "Assertion failure: !win->IsDying()" with long-script interrupt and window.close()

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

Created attachment 8765741 [details]
testcase (manual version - click two buttons)

1. Set the prefs mentioned in the testcase
2. Load the testcase
3. Click two buttons

Result: 
Assertion failure: !win->IsDying(), at /Users/jruderman/trees/mozilla-central/js/xpconnect/src/XPCJSRuntime.cpp:1456

Expected:
script terminated by timeout

Comment 1

[Jesse Ruderman]

Created attachment 8765742 [details]
testcase (automated version)

Comment 2

[Jesse Ruderman]

I can't reproduce when running Firefox under lldb. Does running under lldb disable the script-timeout feature??

Comment 3

[Jesse Ruderman]

Created attachment 8765743 [details]
stack

Comment 4

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

This callback runs arbitrary code, so asserting that the window is not dying seems bogus.  Bobby?

Comment 5

[Bobby Holley (:bholley)]

Yeah, we should probably just check and bail out like we do for the non-window case, though probably in this case we should return false and kill the script.

r=me on a patch that replaces the assert with:

if (win->IsDying()) {
  // The window is being torn down. When that happens we try to prevent the dispatch of new runnables,
  // so it also makes sense to kill any long-running script. The user is primarily interested in this
  // page going away.
  return false;
}

Comment 6

[Hsin-Yi Tsai [:hsinyi]]

Hi Ben, would you please help out here?

Comment 7

[Ben Hsu [:HoPang]]

Sure, but I am new to this field, it would take me a while, and thus I will working on this without taking it :)

Comment 8

[Ben Hsu [:HoPang]]

Hi Jesse, and sorry for such a late response

After diving into the code a little bit, I found that the case is still reproducible by executing firefox in non-e10s mode, which you can do that by the following command. 

```
./mach run --disable-e10s
```

With e10s enabled, it seems that Gecko doesn't immediately close the nsGlobalWindow after invoking `window.close()`, and thus the nsGlobalWindow is not marked as dying. As a result, we don't get hit by the assertion. Please refer to http://searchfox.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#8607-8609 for more detailed information :)

Comment 9

[Ben Hsu [:HoPang]]

Created attachment 8800501 [details] [diff] [review]
Loose the assertion checking win->IsDying()

Comment 10

[Ben Hsu [:HoPang]]

Comment on attachment 8800501 [details] [diff] [review]
Loose the assertion checking win->IsDying()

Per comment 5, I set the flag to r+

Comment 11

[Ben Hsu [:HoPang]]

Comment on attachment 8800501 [details] [diff] [review]
Loose the assertion checking win->IsDying()

Per comment 5, I set this patch to r+

Comment 12

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aec25af40696

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba66a8bb5b3d
Loose the assertion checking win->IsDying(). r=bholly

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba66a8bb5b3d