1276366 - Remove support for chrome -> chrome window leaks

Status: UNCONFIRMED

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

Description

[Andrew McCreight [:mccr8]]

It is very easy for chrome JS to leak a JS window. For our own code, we do test against this, so we usually catch these errors (but not always: see bug 1276271). For addons, of course, we have almost no testing, so there can be leaks of windows even with basic operations (see bug 873163). In theory, we could extend the hueyfix to work for reference to chrome windows also. All it will take is a hero to dig through the failures, and hope there's no systematic problems in chrome code like those that bogged down bug 1084626.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Also nuke chrome window references.

Be dazzled by my coding prowess!

Here's a fresh try run that any curious people can pick through in a few hours:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=da4876482348

Comment 2

[Andrew McCreight [:mccr8]]

I was calling this the "SuperHueyfix" as a joking reference to the Super Huey helicopter, but it will probably fix less leaks than the original hueyfix so that seems inaccurate.

Comment 3

[Andrew McCreight [:mccr8]]

The change seems to break Marionette somehow, which breaks all reftests. Plain mochitests are green as you'd expect. bc tests look like there are a handful of failures per test suite, which isn't terrible.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> The change seems to break Marionette somehow, which breaks all reftests.

That would be the "blooeyfix", then?

Comment 5

[Eric Rahm [:erahm]]

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

Comment 6

[Eric Rahm [:erahm]]

WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74700e1876828869d64ff970fdcaad5a80a74f6a

Marionette tests are fixed, reftests are fixed, a fair amount of e10s mochitests are fixed. Still plenty of non-e10s to work on.

Comment 7

[Eric Rahm [:erahm]]

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

Comment 8

[Eric Rahm [:erahm]]

Attachment: WIP: super-huey-fix-fix for tests

This is essentially a roll-up of what I've been looking at. I'm holding off on
fixing sessionstore and devtools stuff, I think we need to redirect to domain
experts there. I'll split out patches for what I've got and get a list of still
broken tests after the next try run completes.

- mainly focused on e10s, so non-e10s might still be blowing up
- marionette should work now
- devtools was doing some shady stuff with dummy documents in deleted frames,
  keeping the frame around seems to have fixed some things
- fxui tests should be working now
- SessionStore was also doing some sketchy stuff, I think I have workarounds but
- We had some issues in jsm's, so those fixes *should* cascade out and clear
  out a bunch of issues.

Comment 9

[Eric Rahm [:erahm]]

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

Comment 10

[Eric Rahm [:erahm]]

Alright try run from comment 7 looks somewhat better. Basically what's left is session store is super broken (I think basically it's not getting notified that windows are closed) and devtools is a bit sad. Push from comment 9 should fix a fair amount of devtools stuff.

Comment 11

[Eric Rahm [:erahm]]

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

Comment 12

[The 8472]

I suspect that leaking addons would suddenly encounter exceptions or induce exceptions in browser components which don't encounter dead wrappers in vanilla FF. And maybe addon authors won't easily notice if the breakage is subtle.

Is there telemetry for dead wrapper exceptions or something like that to determine the impact?

Comment 13

[Eric Rahm [:erahm]]

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

Comment 14

[Eric Rahm [:erahm]]

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

Comment 15

[Eric Rahm [:erahm]]

Attachment: Part 1: Handle dead windows in test_execute_script

After closing the window it will most likely become a deadwrapper. Jut handle
that gracefully.

We now expect exceptions when trying to run a script on a closed window, so
that means I can't split this change out to a blocking bug.

Comment 16

[Eric Rahm [:erahm]]

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

Comment 17

[David Burns :automatedtester]

Comment on attachment 8786895 [details] [diff] [review]
Part 1: Handle dead windows in test_execute_script

Review of attachment 8786895 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/harness/marionette/tests/unit/test_execute_script.py
@@ +248,5 @@
>          self.marionette.switch_to_window("xul")
>          self.assertNotEqual(self.win, self.marionette.current_window_handle)
>  
>      def tearDown(self):
> +        try:

can you use `self.assertRaises` here instead.

Comment 18

[Eric Rahm [:erahm]]

(In reply to David Burns :automatedtester from comment #17)
> Comment on attachment 8786895 [details] [diff] [review]
> Part 1: Handle dead windows in test_execute_script
> 
> Review of attachment 8786895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/marionette/harness/marionette/tests/unit/test_execute_script.py
> @@ +248,5 @@
> >          self.marionette.switch_to_window("xul")
> >          self.assertNotEqual(self.win, self.marionette.current_window_handle)
> >  
> >      def tearDown(self):
> > +        try:
> 
> can you use `self.assertRaises` here instead.

I'm not confident that it will always raise unfortunately. I guess I can do a try push with that instead.

Comment 19

[David Burns :automatedtester]

(In reply to Eric Rahm [:erahm] from comment #18)
> (In reply to David Burns :automatedtester from comment #17)
> > Comment on attachment 8786895 [details] [diff] [review]
> > Part 1: Handle dead windows in test_execute_script
> > 
> > Review of attachment 8786895 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: testing/marionette/harness/marionette/tests/unit/test_execute_script.py
> > @@ +248,5 @@
> > >          self.marionette.switch_to_window("xul")
> > >          self.assertNotEqual(self.win, self.marionette.current_window_handle)
> > >  
> > >      def tearDown(self):
> > > +        try:
> > 
> > can you use `self.assertRaises` here instead.
> 
> I'm not confident that it will always raise unfortunately. I guess I can do
> a try push with that instead.

just spotted this is in teardown... it's not ideal but I guess we can leave it as you have it.

Comment 20

[Eric Rahm [:erahm]]

Comment on attachment 8757489 [details] [diff] [review]
Also nuke chrome window references.

billm had some opinions on this in Bug 1299589 comment 5, mainly that we might want to wait for session store messages before nuking the compartment. Bill can you elaborate?

This might help with the headache I've been having with various session store tests.

Comment 21

[Eric Rahm [:erahm]]

Attachment: WIP Part 2: more devtools fixes hopefully

Comment 22

[Eric Rahm [:erahm]]

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

Comment 23

[Eric Rahm [:erahm]]

Attachment: WIP Part 3: Session store fixes

Comment 24

[Eric Rahm [:erahm]]

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

Comment 25

[Eric Rahm [:erahm]]

Attachment: WIP: fix widget bustage

Comment 26

[Eric Rahm [:erahm]]

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

Comment 27

[Eric Rahm [:erahm]]

Attachment: WIP: Fix extensions failures

Comment 28

[Eric Rahm [:erahm]]

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

Comment 29

[Eric Rahm [:erahm]]

Attachment: WIP session store fixes pt2: Handle dead wrappers in SessionStoreInternal

This is an attempt to handle possibly dead browser windows in |onClose|. It probably breaks things.

Comment 30

[Eric Rahm [:erahm]]

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

Comment 31

[Eric Rahm [:erahm]]

Okay so I split out a bunch of blockers that can be landed independently, I've split out my updated WIP patches investigating failures and started a try run after each one hoping to be able to detect if any of the fixes actually add more failures.

Next step I'll file blockers for the remaining test failures along with any debug patches I have for them.

Comment 32

[Eric Rahm [:erahm]]

Current issues:

bc1
===
- toolkit/mozapps/extensions/test/browser/browser_cancelCompatCheck.js fails, doesn't cleanup, breaks everything else. I'll file a bug for this, I tried to track down the issue but am at a loss

bc2
===
- session store is rather broken, looks like possible a bug in my devtools patch

bc4
===
- browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js is timing out

cl, dt*
==
- devtools is busted (missing Cu from devtools patch)

Spoke to devtools folks, they'd prefer to avoid using Cu (they're transition their tools to content). I'll update that WIP with try/catch blocks. Hopefully we'll get a greener run as well.

Comment 33

[Eric Rahm [:erahm]]

Attachment: WIP Part 2: more devtools fixes hopefully

Swapping out some Cu.isDeadWrapper usage with try/catch

Comment 34

[Eric Rahm [:erahm]]

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

Comment 35

[Eric Rahm [:erahm]]

Alright, it's going to be hard to make any progress on devtools bustage until bug 1288835 (the create a dummy element in a window and then destroy that window but rely on the dummy element still being all good behavior) is fixed and that's waiting on bug 1265796 which in theory is being worked on.

Pursuing session store failures is also probably not worthwhile until we get feedback from billm.

Comment 36

[Eric Rahm [:erahm]]

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

Comment 37

[Eric Rahm [:erahm]]

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

Comment 38

[Eric Rahm [:erahm]]

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

Comment 39

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: nuking blocker patch

Hi Eric,
This is the solution we discussed for session store code. Here's the process I used to validate it:

1. Force session restore to take a little while to ensure that it loses the race with window nuking. To do that, I added a delay here:
http://searchfox.org/mozilla-central/source/browser/components/sessionstore/content/content-sessionStore.js#205
You may find this useful for testing. With this change (and your patch), session store does not save sessions on normal shutdown.

2. Then I applied the patch here. It waits to nuke the window until we're done doing session store stuff. This causes session store to work again.

I think this approach should work for a variety of other frontend stuff that needs to touch the window after it's closed.

Comment 40

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8757489 [details] [diff] [review]
Also nuke chrome window references.

Cancelling feedback. Please needinfo me or something if you need any more information. I'm happy to review this once it's closer to passing tests.

Comment 41

[Eric Rahm [:erahm]]

Comment on attachment 8797291 [details] [diff] [review]
nuking blocker patch

Review of attachment 8797291 [details] [diff] [review]:
-----------------------------------------------------------------

Bill thanks this is super helpful! I have a few questions below about threading/races.

::: browser/components/sessionstore/SessionStore.jsm
@@ +1330,2 @@
>        TabStateFlusher.flushWindow(aWindow).then(() => {
> +        domWindowUtils.removeNukeBlocker();

Is it possible that this won't get hit? Can flushWindow fail?

::: dom/base/nsGlobalWindow.cpp
@@ +8937,5 @@
> +        nsCOMPtr<nsISupportsPRUint64> wrapper =
> +          do_CreateInstance(NS_SUPPORTS_PRUINT64_CONTRACTID);
> +        if (wrapper) {
> +          wrapper->SetData(mID);
> +          observerService->NotifyObservers(wrapper, mTopic.get(), nullptr);

Is |observerService->NotifyObservers| a synchronous call? If not should be be concerned about getting down to line 8963 before the session store observer is notified?

@@ +8988,5 @@
>  
>  void
> +nsGlobalWindow::AddNukeBlocker()
> +{
> +  MOZ_ASSERT(IsOuterWindow());

Is this always main thread only? Or more specifically do we know it's only accessed from one thread?

@@ +8995,5 @@
> +
> +void
> +nsGlobalWindow::RemoveNukeBlocker()
> +{
> +  MOZ_ASSERT(IsOuterWindow());

I suppose we'd want to assert that mNukeBlockers > 0

@@ +8998,5 @@
> +{
> +  MOZ_ASSERT(IsOuterWindow());
> +  mNukeBlockers--;
> +  if (!mNukeBlockers) {
> +    nsCOMPtr<nsIRunnable> runnable = new WindowDestroyedEvent(this, mWindowID, "");

Just to be clear, the empty topic indicates means we're not going to notify observers (of which I guess session store would have been one, which is what blocked nuking in the first place). Is that right? 

In that case is the flow essentially: we already notified observers, blocked nuking in our observer, and are just continuing cleanup using the logic in the |WindowDestroyedEvent|

Comment 42

[Eric Rahm [:erahm]]

ni for the is "|observerService->NotifyObservers| a synchronous call?" question.

Comment 43

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Eric Rahm [:erahm] from comment #41)
> ::: browser/components/sessionstore/SessionStore.jsm
> @@ +1330,2 @@
> >        TabStateFlusher.flushWindow(aWindow).then(() => {
> > +        domWindowUtils.removeNukeBlocker();
> 
> Is it possible that this won't get hit? Can flushWindow fail?

It seems possible, although the consequences of it failing are kind of bad (probably worse than not nuking the window).

> ::: dom/base/nsGlobalWindow.cpp
> @@ +8937,5 @@
> > +        nsCOMPtr<nsISupportsPRUint64> wrapper =
> > +          do_CreateInstance(NS_SUPPORTS_PRUINT64_CONTRACTID);
> > +        if (wrapper) {
> > +          wrapper->SetData(mID);
> > +          observerService->NotifyObservers(wrapper, mTopic.get(), nullptr);
> 
> Is |observerService->NotifyObservers| a synchronous call? If not should be
> be concerned about getting down to line 8963 before the session store
> observer is notified?

It is synchronous.

> @@ +8988,5 @@
> >  
> >  void
> > +nsGlobalWindow::AddNukeBlocker()
> > +{
> > +  MOZ_ASSERT(IsOuterWindow());
> 
> Is this always main thread only? Or more specifically do we know it's only
> accessed from one thread?

I guess we can assert. Certainly the session store code never runs off the main thread, and nothing in nsIDOMWindowUtils can be used off the main threa.d

> @@ +8995,5 @@
> > +
> > +void
> > +nsGlobalWindow::RemoveNukeBlocker()
> > +{
> > +  MOZ_ASSERT(IsOuterWindow());
> 
> I suppose we'd want to assert that mNukeBlockers > 0

Sure.

> @@ +8998,5 @@
> > +{
> > +  MOZ_ASSERT(IsOuterWindow());
> > +  mNukeBlockers--;
> > +  if (!mNukeBlockers) {
> > +    nsCOMPtr<nsIRunnable> runnable = new WindowDestroyedEvent(this, mWindowID, "");
> 
> Just to be clear, the empty topic indicates means we're not going to notify
> observers (of which I guess session store would have been one, which is what
> blocked nuking in the first place). Is that right? 

Not exactly. This is notifying a different observer topic ("outer-window-destroyed" I think) than session store uses. Session store was already notified of "domwindowclose" some time ago.

We just happen to be using the same runnable for both nuking and "*-window-destroyed". It might be cleaner to split it into two runnables. I was being lazy.

> In that case is the flow essentially: we already notified observers, blocked
> nuking in our observer, and are just continuing cleanup using the logic in
> the |WindowDestroyedEvent|

The flow is like this:
1. We notify domwindowclose. At this point we block nuking.
2. Some time later we notify "outer-window-destroyed" (or maybe "inner-") and try to nuke. We don't because of the block.
3. Later on, all the session store promises get resolved and we remove the blocker.
4. That triggers another runnable, which skips notifying the observer since we already did that the first time. It does the nuking.

Comment 44

[Eric Rahm [:erahm]]

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

Comment 45

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #44)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5942c8b40d24

This seems a bit better, there are a bunch of bc timeouts, I believe that's due to bug 1299589 not being applied (in theory attachment 8757489 [details] [diff] [review] should have made that bug not necessary). There's a few dt failures, mainly from cleanup functions trying to cleanup event handlers on windows that are already dead.

Comment 46

[Eric Rahm [:erahm]]

Latest push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d87a0243dfdf

With the patch from bug 1299589 and some more devtools patches we're looking good. Only two devtools failures, one is an intermittent that might have gotten worse (bug 1280726) that I've gone ahead and fixed, the other is in 'test_inspector-dead-nodes.html' [1] which is a rather odd test. I think it's checking that things in devtools don't blow up when touching dead nodes, but now the test itself blows up.

We still have a few bc1, bc3, bc6 timeouts:
- bc1:
  - browser_cancelCompatCheck.js 
  - browser_checkAddonCompatibility.js
- bc3:
  - browser_privatebrowsing_windowtitle.js
- bc6:
  - browser_644409-scratchpads.js, this times out but leaves an event handler around that makes every subsequent test fail. I can't repro this locally and it doesn't repro in taskcluster so using a one-click loaner won't help

In debug there's also:
  - browser_pageInfo_plugins.js


[1] https://dxr.mozilla.org/mozilla-central/rev/42c95d88aaaa7c2eca1d278399421d437441ac4d/devtools/server/tests/mochitest/test_inspector-dead-nodes.html

Comment 47

[Eric Rahm [:erahm]]

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

Comment 48

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #47)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=23f53fbdbde7

Disabling 'browser_644409-scratchpads.js' allowed the rest of the bc6 tests to run, we look good there.

One more failure noted in debug:
  - browser_394759_behavior.js

Comment 49

[Eric Rahm [:erahm]]

Further triage:

- browser_privatebrowsing_windowtitle.js
  - Still seems to be issues with |BrowserTestUtils.closeWindow| (what we were working on in bug 1299589)
  - Seems to be known orange, bug 1290593, but is now perma-orange
- browser_cancelCompatCheck.js
  - Can't repro locally, logs indicate some callbacks are invalid. It's not terribly useful as I don't know where the callbacks come from. It's possible this could be a candidate for nuke blocking:

>  [task 2016-10-05T18:48:37.354451Z] 18:48:37     INFO -  490 INFO Console message: 1475693234920      addons.manager  WARN    AddonListener threw exception when calling onPropertyChanged: TypeError: can't access dead object (resource://gre/modules/AddonManager.jsm:1757:1) JS Stack trace: [e-mail]:1757:1 < [e-mail]:3076:5 < [e-mail]:5026:7 < [e-mail]:390:7 < [e-mail]:6857:7 < [e-mail]:684:9 < UpdateParser/<@AddonUpdateChecker.jsm:592:49 < EventListener.handleEvent*[e-mail]:592:5 < [e-mail]:931:12 < [e-mail]:6796:18 < [e-mail]:7683:5 < gVersionInfoPage.onPageShow<@update.js:225:7 < [e-mail]:322:42 < [e-mail]:937:23 < [e-mail]:816:7 < Promise*[e-mail]:747:11 < [e-mail]:779:7 < [e-mail]:714:7 < promise_open_compatibility_window/<@browser_cancelCompatCheck.js:175:5 < EventListener.handleEvent*[e-mail]:652:5 < AddonInterpositionService.prototype.interposeProperty/[e-mail]:160:52 < promise_open_compatibility_window@browser_cancelCompatCheck.js:158:3 < cancel_during_repopulate@browser_cancelCompatCheck.js:270:28 < [e-mail]:322:42 < [e-mail]:937:23 < [e-mail]:816:7 < Promise*[e-mail]:747:11 < [e-mail]:779:7 < [e-mail]:714:7 < get _worker/[e-mail]:231:9 < EventHandlerNonNull*get [e-mail]:217:5 < [e-mail]:291:9 < [e-mail]:322:42 < [e-mail]:937:23 < [e-mail]:816:7 < Promise*[e-mail]:747:11 < [e-mail]:779:7 < [e-mail]:454:5 < [e-mail]:220:5 < this.DeferredSave.prototype._startTimer/<@DeferredSave.jsm:175:40 < [e-mail]:1567:5 < [e-mail]:1654:21 < [e-mail]:2018:23 < [e-mail]:3822:34 < [e-mail]:2808:25 < [e-mail]:236:12 < [e-mail]:789:5 < [e-mail]:973:9 < [e-mail]:3017:5 < [e-mail]:71:9
>  [task 2016-10-05T18:48:37.369973Z] 18:48:37     INFO -  491 INFO Console message: 1475693234941      addons.manager  WARN    AddonListener threw exception when calling onEnabling: TypeError: can't access dead object (resource://gre/modules/AddonManager.jsm:1757:1) JS Stack trace: [e-mail]:1757:1 < [e-mail]:3076:5 < [e-mail]:5051:9 < [e-mail]:390:7 < [e-mail]:6857:7 < [e-mail]:684:9 < UpdateParser/<@AddonUpdateChecker.jsm:592:49 < EventListener.handleEvent*[e-mail]:592:5 < [e-mail]:931:12 < [e-mail]:6796:18 < [e-mail]:7683:5 < gVersionInfoPage.onPageShow<@update.js:225:7 < [e-mail]:322:42 < [e-mail]:937:23 < [e-mail]:816:7 < Promise*[e-mail]:747:11 < [e-mail]:779:7 < [e-mail]:714:7 < promise_open_compatibility_window/<@browser_cancelCompatCheck.js:175:5 < EventListener.handleEvent*[e-mail]:652:5 < AddonInterpositionService.prototype.interposeProperty/[e-mail]:160:52 < promise_open_compatibility_window@browser_cancelCompatCheck.js:158:3 < cancel_during_repopulate@browser_cancelCompatCheck.js:270:28 < [e-mail]:322:42 < [e-mail]:937:23 < [e-mail]:816:7 < Promise*[e-mail]:747:11 < [e-mail]:779:7 < [e-mail]:714:7 < get _worker/[e-mail]:231:9 < EventHandlerNonNull*get [e-mail]:217:5 < [e-mail]:291:9 < [e-mail]:322:42 < [e-mail]:937:23 < [e-mail]:816:7 < Promise*[e-mail]:747:11 < [e-mail]:779:7 < [e-mail]:454:5 < [e-mail]:220:5 < this.DeferredSave.prototype._startTimer/<@DeferredSave.jsm:175:40 < [e-mail]:1567:5 < [e-mail]:1654:21 < [e-mail]:2018:23 < [e-mail]:3822:34 < [e-mail]:2808:25 < [e-mail]:236:12 < [e-mail]:789:5 < [e-mail]:973:9 < [e-mail]:3017:5 < [e-mail]:71:9
>  [task 2016-10-05T18:48:37.392145Z] 18:48:37     INFO -  499 INFO Console message: 1475693235041      addons.manager  WARN    AddonListener threw exception when calling onEnabled: TypeError: can't access dead object (resource://gre/modules/AddonManager.jsm:1757:1) JS Stack trace: [e-mail]:1757:1 < [e-mail]:3076:5 < [e-mail]:5071:11 < [e-mail]:390:7 < [e-mail]:6857:7 < [e-mail]:684:9 < UpdateParser/<@AddonUpdateChecker.jsm:592:49 < EventListener.handleEvent*[e-mail]:592:5 < [e-mail]:931:12 < [e-mail]:6796:18 < [e-mail]:7683:5 < gVersionInfoPage.onPageShow<@update.js:225:7 < [e-mail]:322:42 < [e-mail]:937:23 < [e-mail]:816:7 < Promise*[e-mail]:747:11 < [e-mail]:779:7 < [e-mail]:714:7 < promise_open_compatibility_window/<@browser_cancelCompatCheck.js:175:5 < EventListener.handleEvent*[e-mail]:652:5 < AddonInterpositionService.prototype.interposeProperty/[e-mail]:160:52 < promise_open_compatibility_window@browser_cancelCompatCheck.js:158:3 < cancel_during_repopulate@browser_cancelCompatCheck.js:270:28 < [e-mail]:322:42 < [e-mail]:937:23 < [e-mail]:816:7 < Promise*[e-mail]:747:11 < [e-mail]:779:7 < [e-mail]:714:7 < get _worker/[e-mail]:231:9 < EventHandlerNonNull*get [e-mail]:217:5 < [e-mail]:291:9 < [e-mail]:322:42 < [e-mail]:937:23 < [e-mail]:816:7 < Promise*[e-mail]:747:11 < [e-mail]:779:7 < [e-mail]:454:5 < [e-mail]:220:5 < this.DeferredSave.prototype._startTimer/<@DeferredSave.jsm:175:40 < [e-mail]:1567:5 < [e-mail]:1654:21 < [e-mail]:2018:23 < [e-mail]:3822:34 < [e-mail]:2808:25 < [e-mail]:236:12 < [e-mail]:789:5 < [e-mail]:973:9 < [e-mail]:3017:5 < [e-mail]:71:9

- browser_checkAddonCompatibility.js
  - Can't reproduce locally, this could just be fallout from browser_cancelCompatCheck.js failing

- browser_394759_behavior.js
  - This is classic poking a dead object, my guess not related to that test. PageThumbs is probably just doing a periodic thing and holding a busted ref to a window:

> A promise chain failed to handle a rejection:  - at resource://gre/modules/PageThumbs.jsm:421 - TypeError: can't access dead object
> INFO -  Stack trace:
> INFO - PageThumbs_captureAndStoreIfStale/<@resource://gre/modules/PageThumbs.jsm:421:1

Comment 50

[Nicholas Nethercote [inactive]]

mccr8: the code in nsGlobalWindow.cpp has changed a little; there's now some add-on stuff involved. Is removing the "&& !js::IsSystemCompartment(js::GetObjectCompartment(obj))" test still the right thing to do?

There's also a comment "We only want to nuke wrappers for the chrome->content case" that looks like it should be removed or changed.

Comment 51

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #50)
> mccr8: the code in nsGlobalWindow.cpp has changed a little; there's now some
> add-on stuff involved. Is removing the "&&
> !js::IsSystemCompartment(js::GetObjectCompartment(obj))" test still the
> right thing to do?

Hmm, yes, I think so.

> There's also a comment "We only want to nuke wrappers for the
> chrome->content case" that looks like it should be removed or changed.

Yeah, I think it should be more like "We don't want to nuke content->content wrappers".

Comment 52

[Eric Rahm [:erahm]]

I haven't worked on this in a while, we think switching to web extensions only in Firefox 57 might make this a non-issue so I haven't pursued it further.

dev-tools has gone full html so that might unblock a lot of bustage. I think session restore is the largest sticking point.

Comment 53

[u597032]

Is this bug still an issue?

Comment 54

[Andrew McCreight [:mccr8]]

Yes.

Comment 55

[Worcester12345]

Should the "tracking flag" be updated?