Closed Bug 1282671 Opened 6 years ago Closed 5 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 --- affected
firefox52 --- fixed

People

(Reporter: jruderman, Assigned: bhsu)

Details

(Keywords: assertion, testcase)

Attachments

(4 files)

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
I can't reproduce when running Firefox under lldb. Does running under lldb disable the script-timeout feature??
Attached file stack
This callback runs arbitrary code, so asserting that the window is not dying seems bogus.  Bobby?
Flags: needinfo?(bobbyholley)
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;
}
Flags: needinfo?(bobbyholley)
Priority: -- → P2
Hi Ben, would you please help out here?
Flags: needinfo?(bhsu)
Sure, but I am new to this field, it would take me a while, and thus I will working on this without taking it :)
Flags: needinfo?(bhsu)
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 :)
Assignee: nobody → bhsu
Comment on attachment 8800501 [details] [diff] [review]
Loose the assertion checking win->IsDying()

Per comment 5, I set the flag to r+
Attachment #8800501 - Flags: review+
Comment on attachment 8800501 [details] [diff] [review]
Loose the assertion checking win->IsDying()

Per comment 5, I set this patch to r+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba66a8bb5b3d
Loose the assertion checking win->IsDying(). r=bholly
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba66a8bb5b3d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.