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

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM
P2
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Jesse Ruderman, Assigned: HoPang)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla52
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 affected, firefox52 fixed)

Details

Attachments

(4 attachments)

(Reporter)

Description

a year ago
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
(Reporter)

Comment 1

a year ago
Created attachment 8765742 [details]
testcase (automated version)
(Reporter)

Comment 2

a year ago
I can't reproduce when running Firefox under lldb. Does running under lldb disable the script-timeout feature??
(Reporter)

Comment 3

a year ago
Created attachment 8765743 [details]
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)
(Assignee)

Comment 7

a year ago
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)
(Assignee)

Comment 8

a year ago
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)

Updated

a year ago
Assignee: nobody → bhsu
(Assignee)

Comment 9

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

Comment 10

a year ago
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+
(Assignee)

Comment 11

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

Per comment 5, I set this patch to r+
(Assignee)

Comment 12

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aec25af40696
Keywords: checkin-needed

Comment 13

a year ago
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

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba66a8bb5b3d
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.