1451913 - Crash in nsPIDOMWindowInner::UpdateWebSocketCount

Status: VERIFIED FIXED

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

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is
report bp-8cfa13ab-6352-4ee8-8680-a80ea0180405.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsPIDOMWindowInner::UpdateWebSocketCount dom/base/nsGlobalWindowInner.cpp:2641
1 xul.dll mozilla::dom::WebSocketImpl::Disconnect dom/websocket/WebSocket.cpp:621
2 xul.dll mozilla::dom::WebSocketImpl::DispatchConnectionCloseEvents dom/websocket/WebSocket.cpp:1907
3 xul.dll mozilla::dom::CallDispatchConnectionCloseEvents::Run dom/websocket/WebSocket.cpp:261
4 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1096
5 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:97
6 xul.dll mozilla::ipc::MessagePumpForChildProcess::Run ipc/glue/MessagePump.cpp:301
7 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:319
8 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:299
9 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:157

=============================================================

This crash showed up in the 20180404100127 Nightly. Possibly a regression from bug 1450266. #7 Windows top crash for the April 4 Nightlies.

Comment 1

[Andrew McCreight [:mccr8]]

Ben, could this be a regression from your patch?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

This seems related to the assertions I get with the websocket leak test in bug 1451411.  I think this count updating code has a bug in it.  Since this is low frequency I'd like to see if Andrea can fix bug 1451411 before looking at this.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I guess this used to be infrequent, but is now more frequent.  I'll see if I can fix the activity counter stuff.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I wonder if this is caused by:

1. Site opens websocket
2. Site does document.open()
3. websocket closes

We now rebind DETH objects on document.open(), so we probably end up decrementing the count on the new window while we only ever incremented it on the old window.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow DOMEventTargetHelper classes to indicate whether they require the global to remain active in the background. r=farre

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make WebSocket use DOMEventTargetHelper::ShouldBeActiveInBackground(). r=farre

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make IDB classes use DOMEventTargetHelper::ShouldBeActiveInBackground(). r=asuth

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

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make IDB classes use DOMEventTargetHelper::ShouldBeActiveInBackground(). r=asuth

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

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make IDB classes use DOMEventTargetHelper::ShouldBeActiveInBackground(). r=asuth

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

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow DOMEventTargetHelper classes to indicate when budget throttling is allowed or not. r=farre

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make WebSocket use DOMEventTargetHelper::BudgetThrottlingAllowed(). r=farre

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make IDB classes use DOMEventTargetHelper::BackgroundThrottlingAllowed(). r=asuth

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8966354 [details] [diff] [review]
P1 Allow DOMEventTargetHelper classes to indicate when budget throttling is allowed or not. r=farre

Andreas, this patch adds a system by which DOMEventTargetHelper (DETH) objects attached to a global can advertise that they should allow or disallow budget throttling.  The intention is to replace the counters we try to track on the window with code that queries the set of DETH objects.  This will avoid the problem of state inconsistency in weird corner cases.

Note, this will make figuring out "can I budget throttle" more expensive.  It has to query the tree of nested iframes and each DETH object in each window.  Personally I don't think scheduling timers should be that hot a path for most workloads, so this is probably ok.  If it becomes a problem we can add in some caching, etc.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8966355 [details] [diff] [review]
P2 Make WebSocket use DOMEventTargetHelper::BudgetThrottlingAllowed(). r=farre

This patch makes WebSocket use the new DETH::BudgetThrottlingAllowed() mechanism.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8966356 [details] [diff] [review]
P3 Make IDB classes use DOMEventTargetHelper::BackgroundThrottlingAllowed(). r=asuth

Andrew, this patch converts IDB code over to use the new BudgetThrottlingAllowed() mechanism introduced in P1 of this bug.  The goal is to allow the global to query its event targets' current state to determine if throttling is allowed instead of trying to track counters in the window.  This will automatically handle various corner cases like document.open() gracefully.

Note, its not strictly necessary to convert IDB over in this bug, but I wanted to do it for a couple reasons:

1. IDB has a test for throttling behavior and WebSocket does not.  So doing this lets us exercise the P1 patch in automation.
2. It proves that the P1 patch is suitable to handle multiple features.

Note, I did not convert over HasActivePeerConnections() because that method is called in another circumstance beyond TimeoutManager.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8966356 [details] [diff] [review]
P3 Make IDB classes use DOMEventTargetHelper::BackgroundThrottlingAllowed(). r=asuth

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

This is a fantastic cleanup, thank you!  I've filed bug 1452917 to do a little more cleanup/potential optimization in IDB.  Please note that there is one necessary action item, it's not all restatings.

Restating the pre-this-patch-world:
- We were concerned about throttled background tabs blocking foreground tabs because write transactions block other write transactions/overlapping-scope read transactions, plus open databases older versions block version upgrades.
- nsGlobalWindowInner tracked the number of active databases and active transactions.
  - Active databases also includes the open requests (IDBOpenDBRequest) because the request will hit IPC and cause upgrade problems regardless of the global knowing about the resulting IDBDatabase.
- nsGlobalWindowInner would only use the number of active databases to forbid budget throttling because it's a superset of coverage; you can't have an active transaction without an active database.  The transaction count was intended to support Quantum DOM Scheduling.  I don't think it was ever used, but it's certainly no longer used, and bug 1452917 covers the removal of the IDB bits if the Quantum Scheduling removal doesn't remove that entirely.

Restating post-patch:
- We now also will consider the transaction state for budget throttling where we wouldn't previously, but that's fine, that was just an optimization since there must be an active database for there to be an active transaction.

::: dom/indexedDB/IDBDatabase.cpp
@@ -1149,5 @@
>  
>  void
> -IDBDatabase::NoteInactiveTransactionDelayed()
> -{
> -  ExpireFileActors(/* aExpireAll */ false);

Unfortunately, I think this is a load-bearing call to ExpireFileActors(false).  While ExpireFileActors(false) simply scans mFileActors for moot weak-references and deletes the corresponding parent-actor[1], the only other time we call this in steady-state is under memory-pressure.  So unless you want to hook ExpireFileActors up to a post-GC hook or something like that, we need a call to this from the current IDBTransaction::MaybeNoteInactiveTransaction somehow.

The good news is the runnable delay isn't actually necessary, it looks like Bevis was concerned about pre-emption points, but that was a Quantum Scheduler thing.

The bad news is that ExpireFileActors is private, so maybe the simplest thing is to keep NoteInactiveTransaction around but have it only call the expire method.  You don't need to guard the call with any conditionals, though.

1: This is part of IDB's Blob/File de-duplication magic by remembering Blobs we've seen before.

::: dom/indexedDB/IDBDatabase.h
@@ +266,2 @@
>      mBackgroundActor = nullptr;
> +    CloseInternal();

Restating: This change was made after asking me on IRC about whether this was necessary and I concluded that we'd potentially crash at https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/dom/indexedDB/IDBDatabase.cpp#715 in IDBDatabase::Transaction if we didn't enter the closed state.  Also, that the crash would be very rare since we don't normally expect the parent actor to go away without the child requesting it.  It shouldn't happen in shutdown, but might be possible during privacy-related origin-clearings.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Sutherland [:asuth] from comment #17)
> ::: dom/indexedDB/IDBDatabase.cpp
> @@ -1149,5 @@
> >  
> >  void
> > -IDBDatabase::NoteInactiveTransactionDelayed()
> > -{
> > -  ExpireFileActors(/* aExpireAll */ false);

Ack.  This was unintentional.  I'll add it back.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8966354 [details] [diff] [review]
P1 Allow DOMEventTargetHelper classes to indicate when budget throttling is allowed or not. r=farre

Actually, I'm not sure these changes are adequate.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

The root problem here is that there are various stateful things on the inner window tied to the DETH objects attached to it.  When document.open() re-binds the DETH objects to the new inner window we are losing this current state.  When the new window closes we then hit our assertions about cleaning up state nicely.

I think we need to make a DETH::OnBind() virtual method hook DETH classes can implement.  This will allow them to move any inner window state from the old window to the new window.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Delay rebinding and freeing the old inner window until after set the new inner window. r=smaug

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add DOMEventTargetHelper::OnDynamicBind() method. r=smaug

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Update websocket state on the inner window when rebinding. r=smaug

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make AudioContext add itself to the new window when its rebound for a document.open(). r=smaug

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

I still need to fix webrtc and IDB.  Unfortunately IDB will require a special snowflake fix.

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

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

We probably need to update the window-to-worker map on document.open() as well.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Delay rebinding and freeing the old inner window until after set the new inner window. r=smaug

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Don't manually create a new Performance object on document.open() since we are rebinding the old Performance to the new global. r=smaug

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Migrate the IDBFactory to the new window on document.open(). r=smaug

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Migrate the IDBFactory to the new window on document.open(). r=smaug

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Test that WebSocket does not leak through its event listeners. r=smaug

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Add an IDB event target leak test. r=asuth

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Currently the #4 top content process crash.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Add an IDB event target leak test. r=asuth

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Add an AudioContext event target leak test. r=smaug

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

I still need to fixup workers on document.open(), but lets see where this leaves us:

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

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

It seems I broke test_bug372964-2.html again in this patch queue somewhere.

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #38)
> It seems I broke test_bug372964-2.html again in this patch queue somewhere.

Ah, its in P0:

  currentInner->MigrateStateForDocumentOpen(newInnerWindow);

Should be:

  newInnerWindow->MigrateStateForDocumentOpen(currentInner);

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Delay rebinding and freeing the old inner window until after set the new inner window. r=smaug

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Add an IDB event target leak test. r=asuth

*shakes fist at eslint*

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Add an IDB event target leak test. r=asuth

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Migrate the IDBFactory to the new window on document.open(). r=smaug

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

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

I think I'm going to punt on the worker stuff for now.  I've verified workers don't crash on document.open().  Instead we end up calling CancelWorkersForWindow() from FreeInnerObjects() which stops the workers.  This seems ok for now.

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Delay rebinding and freeing the old inner window until after set the new inner window. r=smaug

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add DOMEventTargetHelper::OnDynamicBind() method. r=smaug

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Update websocket state on the inner window when rebinding. r=smaug

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make AudioContext add itself to the new window when its rebound for a document.open(). r=smaug

Comment 50

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Don't manually create a new Performance object on document.open() since we are rebinding the old Performance to the new global. r=smaug

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Migrate the IDBFactory to the new window on document.open(). r=smaug

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Test that WebSocket does not leak through its event listeners. r=smaug

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Add an IDB event target leak test. r=smaug

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P8 Add an AudioContext event target leak test. r=smaug

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8967455 [details] [diff] [review]
P0 Delay rebinding and freeing the old inner window until after set the new inner window. r=smaug

Olli, the root problem I'm trying to fix in this bug is that many APIs stash state on the inner window and loses access to it when we rebind to the new window for a document.open().

This patch adds a new method on the inner window that will be called for document.open() to migrate state from the old window to the new window.  This includes the re-binding of the DETH objects.

The patch also moves the state migration and FreeInnerObjects() call down in SetNewDocument().  Some of the state migration code requires the new window to be somewhat fully setup so it can access things like mTopWindow and mDoc, etc.

Note, my hope is that one day we can implement bug 1449992 and this entire mechanism can go away.  Working on this has convinced me that reusing the window on document.open() is much more sane than trying to keep all the different kind of binding objects working after we swap in a new inner window.

Comment 56

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8967456 [details] [diff] [review]
P1 Add DOMEventTargetHelper::OnDynamicBind() method. r=smaug

This patch adds an OnDynamicBind() virtual method hook to the DETH base class.  Its called when we rebind the object to a new global.  This will be used in later patches to update state on the global.

Note, I wish I could have just always called this method when we bind the global, but I can't call a virtual method from a base class constructor.

Comment 57

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8967457 [details] [diff] [review]
P2 Update websocket state on the inner window when rebinding. r=smaug

This patch uses the OnDynamicBind() hook to transfer websocket activity state from the old window to the new window.

Comment 58

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8967458 [details] [diff] [review]
P3 Make AudioContext add itself to the new window when its rebound for a document.open(). r=smaug

This patch moves the AudioContext weak reference from the old window to the new window.

Comment 59

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8967459 [details] [diff] [review]
P4 Don't manually create a new Performance object on document.open() since we are rebinding the old Performance to the new global. r=smaug

This patch removes the old document.open() code that created a new Performance object using the old object's state.  Now that we are rebinding, lets just keep the original object.

Comment 60

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8967460 [details] [diff] [review]
P5 Migrate the IDBFactory to the new window on document.open(). r=smaug

This patch moves the IDB state from the old window to the new window.  Note, IDB objects do extend DETH, but I can't easily use OnDynamicRebind() here because of how IDB funnels everything through this non-DETH IDBFactory object.

Comment 61

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8967461 [details] [diff] [review]
P6 Test that WebSocket does not leak through its event listeners. r=smaug

Add an event target leak test for WebSocket.  The document.open() code in this test triggered the assertion in comment 0.

Comment 62

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8967462 [details] [diff] [review]
P7 Add an IDB event target leak test. r=smaug

This adds an IDB event target leak test.  This is to make sure we exercise the document.open() code with an active IDB transaction.

Comment 63

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8967463 [details] [diff] [review]
P8 Add an AudioContext event target leak test. r=smaug

Add an AudioContext event target leak test.  This is for coverage of the document.open() code with an active AudioContext.

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8967456 [details] [diff] [review]
P1 Add DOMEventTargetHelper::OnDynamicBind() method. r=smaug

Even after a night this feels awkward. I would just let DETH implementations to override BindToOwner(nsIGlobalObject* aOwner).
To my eyes OnDynamicBind look unneeded addition, when it would be enough to
have virtual BindToOwner, and that way the argument is also what one would expect, the new global, not old one.

Comment 65

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8967463 [details] [diff] [review]
P8 Add an AudioContext event target leak test. r=smaug

+// Its important here that we create a listener callback from
+// the DOM objects back to the frame's global in order to
+// exercise the leak condition.
It's 

rs+


It is unclear to me why we need to manually add 
+  ctx.onstatechange = e => {
+    contentWindow.stateChangeCount += 1;
+  };
Why the leak script couldn't just add event listener which has a reference to the window.

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8967462 [details] [diff] [review]
P7 Add an IDB event target leak test. r=smaug

It's

rs+

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8967457 [details] [diff] [review]
P2 Update websocket state on the inner window when rebinding. r=smaug

This would use BindToOwner, I hope, but otherwise ok.

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8967460 [details] [diff] [review]
P5 Migrate the IDBFactory to the new window on document.open(). r=smaug

Ahaa,IDBFactory is not a DETH.
I wonder if we should move BindToOwner stuff out from DETH to some helper class.

but r+ anyhow

(This all is scary stuff)

Comment 69

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow subclasses to override DOMEventTargetHelper::BindToOwner(nsIGlobalObject*). r=smaug

Comment 70

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Update websocket state on the inner window when rebinding. r=smaug

Comment 71

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make AudioContext add itself to the new window when its rebound for a document.open(). r=smaug

Comment 72

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 73

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Allow subclasses to override DOMEventTargetHelper::BindToOwner(nsIGlobalObject*). r=smaug

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

Comment 74

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Update websocket state on the inner window when rebinding. r=smaug

Helps if you keep the ScopeExit alive till the end of the scope...

Comment 75

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make AudioContext add itself to the new window when its rebound for a document.open(). r=smaug

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

Comment 76

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8967811 [details] [diff] [review]
P1 Allow subclasses to override DOMEventTargetHelper::BindToOwner(nsIGlobalObject*). r=smaug

This looks ok. r+ even if there wasn't '?'

Comment 77

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8967845 [details] [diff] [review]
P2 Update websocket state on the inner window when rebinding. r=smaug

ah, this is quite nice.

Comment 78

[Ben Kelly [:bkelly, not reviewing]]

Thanks.  I was waiting for some try retriggers, but they turned out green.  Also a TV failure I probably need to fix.

Comment 79

[Ben Kelly [:bkelly, not reviewing]]

I retrigged the TV a few times.  If its not perma-orange, then I will land this to get the critical crasher fixed.

Comment 80

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P9 Try to avoid event listener leak test intermittents by doing more GC. r=baku

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

Comment 81

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8968077 [details] [diff] [review]
P9 Try to avoid event listener leak test intermittents by doing more GC. r=baku

Andrea, this patch does even more GC during the event target leak tests.  My hope is this will address some intermittent failures in the document.open() case for existing tests.  It also reduces the frequency of failures in the TV case for the IDB test in this bug.

If we see continued intermittent failure then I'll open another bug to research further.

Comment 82

[Ben Kelly [:bkelly, not reviewing]]

I think maybe I should also immediately disconnect IDBFactory's mWindow ref back to the window when we call FreeInnerObjects().

Comment 83

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P10 Make the inner window explicitly disconnect the IDBFactory. r=asuth

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

Comment 84

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8968085 [details] [diff] [review]
P10 Make the inner window explicitly disconnect the IDBFactory. r=asuth

This further reduces the reproduction of the orange in the --verify TV runs.  There is clearly some additional way we can delay GC or leak in this IDB test, but I don't want to delay landing the critical crash fixes for that.  I'd like to land these two bandaids for now and we can track down any intermittents later.

Comment 85

[Ben Kelly [:bkelly, not reviewing]]

I have to fix the bug number in the last patch.  I'll land later this morning.

Comment 86

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3b3fac9a495
P0 Delay rebinding and freeing the old inner window until after set the new inner window. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/3baa317d82f5
P1 Allow subclasses to override DOMEventTargetHelper::BindToOwner(nsIGlobalObject*). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff0b30a815e
P2 Update websocket state on the inner window when rebinding. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4952f8b12700
P3 Make AudioContext add itself to the new window when its rebound for a document.open(). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f45cea49e22
P4 Don't manually create a new Performance object on document.open() since we are rebinding the old Performance to the new global. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/102a57aaff3a
P5 Migrate the IDBFactory to the new window on document.open(). r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0eed49c93565
P6 Test that WebSocket does not leak through its event listeners. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9240cfa840
P7 Add an IDB event target leak test. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd0da361b21
P8 Add an AudioContext event target leak test. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/76afcfbd047b
P9 Try to avoid event listener leak test intermittents by doing more GC. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca5da952ff4c
P10 Make the inner window explicitly disconnect the IDBFactory. r=baku

Comment 87

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e3b3fac9a495
https://hg.mozilla.org/mozilla-central/rev/3baa317d82f5
https://hg.mozilla.org/mozilla-central/rev/1ff0b30a815e
https://hg.mozilla.org/mozilla-central/rev/4952f8b12700
https://hg.mozilla.org/mozilla-central/rev/7f45cea49e22
https://hg.mozilla.org/mozilla-central/rev/102a57aaff3a
https://hg.mozilla.org/mozilla-central/rev/0eed49c93565
https://hg.mozilla.org/mozilla-central/rev/2f9240cfa840
https://hg.mozilla.org/mozilla-central/rev/8dd0da361b21
https://hg.mozilla.org/mozilla-central/rev/76afcfbd047b
https://hg.mozilla.org/mozilla-central/rev/ca5da952ff4c

Comment 88

[Ben Kelly [:bkelly, not reviewing]]

Its still early, but so far no crash reproductions in build id 20180416220315.

Comment 89

[Ryan VanderMeulen [:RyanVM]]

No crashes since this landed!