Open Bug 1276366 Opened 5 years ago Updated 2 years ago

Remove support for chrome -> chrome window leaks

Categories

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

defect

Tracking

()

UNCONFIRMED
Tracking Status
firefox49 --- affected

People

(Reporter: mccr8, Unassigned)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [MemShrink:P2] btpp-active)

Attachments

(9 files, 1 obsolete file)

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.
Summary: Remove support for chrome -> chrome leaks → Remove support for chrome -> chrome window leaks
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
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.
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.
(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?
Whiteboard: [MemShrink] → [MemShrink:P2]
Whiteboard: [MemShrink:P2] → [MemShrink:P2] btpp-active
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.
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.
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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.
Depends on: 1288831
Depends on: 1288835
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?
Depends on: 1299587
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.
Attachment #8786895 - Flags: review?(dburns)
Depends on: 1299589
Depends on: 1299592
Depends on: 1299596
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.
Attachment #8786895 - Flags: review?(dburns) → review+
Depends on: 1299608
(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.
(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.
Depends on: 1299613
Depends on: 1299626
Depends on: 1299631
Depends on: 1299642
Depends on: 1299645
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.
Attachment #8757489 - Flags: feedback?(wmccloskey)
This is an attempt to handle possibly dead browser windows in |onClose|. It probably breaks things.
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.
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.
Swapping out some Cu.isDeadWrapper usage with try/catch
Attachment #8787019 - Attachment is obsolete: true
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.
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.
Attachment #8797291 - Flags: feedback?(erahm)
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.
Attachment #8757489 - Flags: feedback?(wmccloskey)
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|
Attachment #8797291 - Flags: feedback?(erahm) → feedback+
ni for the is "|observerService->NotifyObservers| a synchronous call?" question.
Flags: needinfo?(wmccloskey)
(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.
Flags: needinfo?(wmccloskey)
(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.
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
(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
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: AddonManagerInternal.callAddonListeners@AddonManager.jsm:1757:1 < this.AddonManagerPrivate.callAddonListeners@AddonManager.jsm:3076:5 < this.XPIProvider.updateAddonDisabledState@XPIProvider.jsm:5026:7 < .applyCompatibilityUpdate@XPIProviderUtils.js:390:7 < UpdateChecker.prototype.onUpdateCheckComplete@XPIProvider.jsm:6857:7 < UpdateParser.prototype.onLoad@AddonUpdateChecker.jsm:684:9 < UpdateParser/<@AddonUpdateChecker.jsm:592:49 < EventListener.handleEvent*UpdateParser@AddonUpdateChecker.jsm:592:5 < this.AddonUpdateChecker.checkForUpdates@AddonUpdateChecker.jsm:931:12 < UpdateChecker@XPIProvider.jsm:6796:18 < AddonWrapper.prototype.findUpdates@XPIProvider.jsm:7683:5 < gVersionInfoPage.onPageShow<@update.js:225:7 < TaskImpl_run@Task.jsm:322:42 < Handler.prototype.process@Promise-backend.js:937:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:816:7 < Promise*this.PromiseWalker.scheduleWalkerLoop@Promise-backend.js:747:11 < this.PromiseWalker.schedulePromise@Promise-backend.js:779:7 < this.PromiseWalker.completePromise@Promise-backend.js:714:7 < promise_open_compatibility_window/<@browser_cancelCompatCheck.js:175:5 < EventListener.handleEvent*EventTargetInterposition.methods.addEventListener@RemoteAddonsParent.jsm:652:5 < AddonInterpositionService.prototype.interposeProperty/desc.value@multiprocessShims.js:160:52 < promise_open_compatibility_window@browser_cancelCompatCheck.js:158:3 < cancel_during_repopulate@browser_cancelCompatCheck.js:270:28 < TaskImpl_run@Task.jsm:322:42 < Handler.prototype.process@Promise-backend.js:937:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:816:7 < Promise*this.PromiseWalker.scheduleWalkerLoop@Promise-backend.js:747:11 < this.PromiseWalker.schedulePromise@Promise-backend.js:779:7 < this.PromiseWalker.completePromise@Promise-backend.js:714:7 < get _worker/worker.onmessage@PromiseWorker.jsm:231:9 < EventHandlerNonNull*get _worker@PromiseWorker.jsm:217:5 < postMessage@PromiseWorker.jsm:291:9 < TaskImpl_run@Task.jsm:322:42 < Handler.prototype.process@Promise-backend.js:937:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:816:7 < Promise*this.PromiseWalker.scheduleWalkerLoop@Promise-backend.js:747:11 < this.PromiseWalker.schedulePromise@Promise-backend.js:779:7 < Promise.prototype.then@Promise-backend.js:454:5 < this.DeferredSave.prototype._deferredSave@DeferredSave.jsm:220:5 < this.DeferredSave.prototype._startTimer/<@DeferredSave.jsm:175:40 < syncLoadManifestFromFile@XPIProvider.jsm:1567:5 < addMetadata@XPIProviderUtils.js:1654:21 < processFileChanges@XPIProviderUtils.js:2018:23 < this.XPIProvider.checkForChanges@XPIProvider.jsm:3822:34 < this.XPIProvider.startup@XPIProvider.jsm:2808:25 < callProvider@AddonManager.jsm:236:12 < _startProvider@AddonManager.jsm:789:5 < AddonManagerInternal.startup@AddonManager.jsm:973:9 < this.AddonManagerPrivate.startup@AddonManager.jsm:3017:5 < amManager.prototype.observe@addonManager.js: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: AddonManagerInternal.callAddonListeners@AddonManager.jsm:1757:1 < this.AddonManagerPrivate.callAddonListeners@AddonManager.jsm:3076:5 < this.XPIProvider.updateAddonDisabledState@XPIProvider.jsm:5051:9 < .applyCompatibilityUpdate@XPIProviderUtils.js:390:7 < UpdateChecker.prototype.onUpdateCheckComplete@XPIProvider.jsm:6857:7 < UpdateParser.prototype.onLoad@AddonUpdateChecker.jsm:684:9 < UpdateParser/<@AddonUpdateChecker.jsm:592:49 < EventListener.handleEvent*UpdateParser@AddonUpdateChecker.jsm:592:5 < this.AddonUpdateChecker.checkForUpdates@AddonUpdateChecker.jsm:931:12 < UpdateChecker@XPIProvider.jsm:6796:18 < AddonWrapper.prototype.findUpdates@XPIProvider.jsm:7683:5 < gVersionInfoPage.onPageShow<@update.js:225:7 < TaskImpl_run@Task.jsm:322:42 < Handler.prototype.process@Promise-backend.js:937:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:816:7 < Promise*this.PromiseWalker.scheduleWalkerLoop@Promise-backend.js:747:11 < this.PromiseWalker.schedulePromise@Promise-backend.js:779:7 < this.PromiseWalker.completePromise@Promise-backend.js:714:7 < promise_open_compatibility_window/<@browser_cancelCompatCheck.js:175:5 < EventListener.handleEvent*EventTargetInterposition.methods.addEventListener@RemoteAddonsParent.jsm:652:5 < AddonInterpositionService.prototype.interposeProperty/desc.value@multiprocessShims.js:160:52 < promise_open_compatibility_window@browser_cancelCompatCheck.js:158:3 < cancel_during_repopulate@browser_cancelCompatCheck.js:270:28 < TaskImpl_run@Task.jsm:322:42 < Handler.prototype.process@Promise-backend.js:937:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:816:7 < Promise*this.PromiseWalker.scheduleWalkerLoop@Promise-backend.js:747:11 < this.PromiseWalker.schedulePromise@Promise-backend.js:779:7 < this.PromiseWalker.completePromise@Promise-backend.js:714:7 < get _worker/worker.onmessage@PromiseWorker.jsm:231:9 < EventHandlerNonNull*get _worker@PromiseWorker.jsm:217:5 < postMessage@PromiseWorker.jsm:291:9 < TaskImpl_run@Task.jsm:322:42 < Handler.prototype.process@Promise-backend.js:937:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:816:7 < Promise*this.PromiseWalker.scheduleWalkerLoop@Promise-backend.js:747:11 < this.PromiseWalker.schedulePromise@Promise-backend.js:779:7 < Promise.prototype.then@Promise-backend.js:454:5 < this.DeferredSave.prototype._deferredSave@DeferredSave.jsm:220:5 < this.DeferredSave.prototype._startTimer/<@DeferredSave.jsm:175:40 < syncLoadManifestFromFile@XPIProvider.jsm:1567:5 < addMetadata@XPIProviderUtils.js:1654:21 < processFileChanges@XPIProviderUtils.js:2018:23 < this.XPIProvider.checkForChanges@XPIProvider.jsm:3822:34 < this.XPIProvider.startup@XPIProvider.jsm:2808:25 < callProvider@AddonManager.jsm:236:12 < _startProvider@AddonManager.jsm:789:5 < AddonManagerInternal.startup@AddonManager.jsm:973:9 < this.AddonManagerPrivate.startup@AddonManager.jsm:3017:5 < amManager.prototype.observe@addonManager.js: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: AddonManagerInternal.callAddonListeners@AddonManager.jsm:1757:1 < this.AddonManagerPrivate.callAddonListeners@AddonManager.jsm:3076:5 < this.XPIProvider.updateAddonDisabledState@XPIProvider.jsm:5071:11 < .applyCompatibilityUpdate@XPIProviderUtils.js:390:7 < UpdateChecker.prototype.onUpdateCheckComplete@XPIProvider.jsm:6857:7 < UpdateParser.prototype.onLoad@AddonUpdateChecker.jsm:684:9 < UpdateParser/<@AddonUpdateChecker.jsm:592:49 < EventListener.handleEvent*UpdateParser@AddonUpdateChecker.jsm:592:5 < this.AddonUpdateChecker.checkForUpdates@AddonUpdateChecker.jsm:931:12 < UpdateChecker@XPIProvider.jsm:6796:18 < AddonWrapper.prototype.findUpdates@XPIProvider.jsm:7683:5 < gVersionInfoPage.onPageShow<@update.js:225:7 < TaskImpl_run@Task.jsm:322:42 < Handler.prototype.process@Promise-backend.js:937:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:816:7 < Promise*this.PromiseWalker.scheduleWalkerLoop@Promise-backend.js:747:11 < this.PromiseWalker.schedulePromise@Promise-backend.js:779:7 < this.PromiseWalker.completePromise@Promise-backend.js:714:7 < promise_open_compatibility_window/<@browser_cancelCompatCheck.js:175:5 < EventListener.handleEvent*EventTargetInterposition.methods.addEventListener@RemoteAddonsParent.jsm:652:5 < AddonInterpositionService.prototype.interposeProperty/desc.value@multiprocessShims.js:160:52 < promise_open_compatibility_window@browser_cancelCompatCheck.js:158:3 < cancel_during_repopulate@browser_cancelCompatCheck.js:270:28 < TaskImpl_run@Task.jsm:322:42 < Handler.prototype.process@Promise-backend.js:937:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:816:7 < Promise*this.PromiseWalker.scheduleWalkerLoop@Promise-backend.js:747:11 < this.PromiseWalker.schedulePromise@Promise-backend.js:779:7 < this.PromiseWalker.completePromise@Promise-backend.js:714:7 < get _worker/worker.onmessage@PromiseWorker.jsm:231:9 < EventHandlerNonNull*get _worker@PromiseWorker.jsm:217:5 < postMessage@PromiseWorker.jsm:291:9 < TaskImpl_run@Task.jsm:322:42 < Handler.prototype.process@Promise-backend.js:937:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:816:7 < Promise*this.PromiseWalker.scheduleWalkerLoop@Promise-backend.js:747:11 < this.PromiseWalker.schedulePromise@Promise-backend.js:779:7 < Promise.prototype.then@Promise-backend.js:454:5 < this.DeferredSave.prototype._deferredSave@DeferredSave.jsm:220:5 < this.DeferredSave.prototype._startTimer/<@DeferredSave.jsm:175:40 < syncLoadManifestFromFile@XPIProvider.jsm:1567:5 < addMetadata@XPIProviderUtils.js:1654:21 < processFileChanges@XPIProviderUtils.js:2018:23 < this.XPIProvider.checkForChanges@XPIProvider.jsm:3822:34 < this.XPIProvider.startup@XPIProvider.jsm:2808:25 < callProvider@AddonManager.jsm:236:12 < _startProvider@AddonManager.jsm:789:5 < AddonManagerInternal.startup@AddonManager.jsm:973:9 < this.AddonManagerPrivate.startup@AddonManager.jsm:3017:5 < amManager.prototype.observe@addonManager.js: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
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(continuation)
(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".
Flags: needinfo?(continuation)
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.
Status: ASSIGNED → NEW
Assignee: erahm → nobody
Status: NEW → UNCONFIRMED
Ever confirmed: false
Priority: -- → P3
Component: DOM → DOM: Core & HTML

Is this bug still an issue?

Yes.

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