1583859 - Refreshing tab when on Slack, crashes the browser

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P1)

Description

[Jonathan Almeida [:jonalmeida]]

STR

1. Log into a Slack group.
2. When the page loads, refresh the page (I used Cmd + R, it that makes a difference).

Expected

• Refresh and don't crash.

Actual

• 💥
• Maybe a separate issue, but sites don't all load doesn't seem to work well.

This doesn't seem to happen on other pages in my brief testing.

Version: 71.0a1 (2019-09-25) (64-bit)
Crash log: https://crash-stats.mozilla.org/report/index/e298aa31-dcba-4308-8f71-7c6850190925

Comment 1

[Jonathan Almeida [:jonalmeida]]

Disabled extensions and the issue stopped showing up. It seems it was related to the fx_cast extension I have which has a native component to it that might be related.

Feel free to close if there is nothing actionable to do on the browser side to avoid these kinds of crashes.

Comment 2

[Jonathan Almeida [:jonalmeida]]

Ignore comment 1, the issue returned after disabling every extension individually. This is not related to any particular addon. Restarting in safe mode (which I had originally tried but didn't mention) seems to solve the problem.

Not sure what the difference is between safe mode and all addons disabled. ¯\_(ツ)_/¯

Comment 3

[Marcia Knous [:marcia]]

Seems similar to Bug 1370005, which was fixed a while back. The moz crash reason is MOZ_DIAGNOSTIC_ASSERT(!IsExclusive || !mHaveRequest) (Using an exclusive promise in a non-exclusive fashion). Not sure if this should move to Core | AV Playback, similar to Bug 1370005.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Dolske, can you help find an owner for this ? Sounds like it's easily reproducible. Or, do you think this should be in a different component?

Comment 5

[Mark Banner (:standard8)]

Hsin-Yi, we think this is more a dom issue, could you take a look at this please (or getting someone to look at it as per comment 4).

Comment 6

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

Not a DOM Core issue. ClientManager is more service worker related. And this isn't happening on the main thread.

Comment 7

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

This looks like another case of GenericPromise (IsExclusive=true) being a bad name and default for an abstraction that imitates an abstraction with no such limitation.

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #7)

This looks like another case of GenericPromise (IsExclusive=true) being a bad name and default for an abstraction that imitates an abstraction with no such limitation.

Does this regress from https://bugzilla.mozilla.org/show_bug.cgi?id=1456995?

Comment 9

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)

Does this regress from https://bugzilla.mozilla.org/show_bug.cgi?id=1456995?

Not directly, no. There could be some level of correlation, but this is Clients API stuff which is largely unaffected by parent-intercept. As such, uplift is likely appropriate.

Comment 10

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

This crash signature is too broad, but I'm not sure how to refine it appropriately. For example, it's also picking up:

• ClientSourceOpChild::DoSourceOp null deref:
• https://crash-stats.mozilla.org/report/index/d5010436-4779-455b-a698-d0f0f0191021
• Media illegal instructions crashes:
  • https://crash-stats.mozilla.org/report/index/b93721b7-09cf-474f-9a52-9cc0e0191019
  • https://crash-stats.mozilla.org/report/index/1d02dca6-18c7-4d71-8b23-5b94d0191019
  • https://crash-stats.mozilla.org/report/index/8c5cb1b0-6aec-444c-83d6-31f310191019
  • etc.

It seems like perhaps the frames that start with "mozilla::MozPromise" should be filtered out? Or at least those with the GenericPromise "T" value giving a prefix of mozilla::MozPromise<bool, nsresult, true>, also the GenericNonExclusivePromise case of mozilla::MozPromise<bool, nsresult, false>. In the media crashes, their template parameters are implementation specific, so could be differentiated on, although they're also fairly chatty. NI'ing :willkg for Socorro.

Comment 11

[Will Kahn-Greene [:willkg] ET needinfo? me]

Andrew: I wrote up bug #1590194 to add mozilla::MozPromise<T>::ThenInternal to the signature prefix list.

Comment 12

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

Attachment: Bug 1583859 - ExecutionReadyPromise should not be exclusive. r=perry

Although many Clients API usages are inherently exclusive (a specific claim
or control request), the execution-ready promise is shared by all requests
to get the state of a client that is not yet execution ready.

Comment 13

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

Attachment: Bug 1583859 - Add GenericExclusivePromise typedef, improve comments. r=bholley

GenericPromise is a somewhat dangerous typedef because its name conceals that
it is exclusive, and an exclusive promise is in contradiction to the presumed
Promise semantics given that all JS Promises are non-exclusive.

This adds a GenericExclusivePromise typedef that's equivalent to the existing
GenericPromise typedef and tries to provide guidance to the existing
GenericPromise comments to use the more explicitly named version.

Depends on D49976

Comment 14

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

Attachment: Bug 1583859 - Convert Clients and ServiceWorkers GenericPromise usage to GenericExclusivePromise. r=perry

GenericExclusivePromise is a more explicitly named GenericPromise. The goal of
this conversion is to make it clear that these uses:

1. Have been audited to some approximation and appear to be correctly and
   intentionally exclusive.
2. Make it clear when we're writing new code that attempts to reuse these
   promises may result in runtime problems. It's easy to gloss over
   the type GenericPromise and not realize that it's exclusive.

In general, the ServiceWorker and Clients usages tend to fall into two usage
patterns:

1. Single-use promise for a specific request. If content code makes multiple
   requests, multiple promises are created.
2. Promises used under the hood, but are converted back to explicit callbacks
   before the crossing into bindings.

Depends on D49977

Comment 15

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

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87c82b01a388
ExecutionReadyPromise should not be exclusive. r=perry

Comment 17

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 87c82b01a388 (Bug 1583859) for causing bustages in /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=272322689

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=272322689&repo=autoland&lineNumber=23269

Comment 18

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2cd1797819f4
Backed out changeset 87c82b01a388 for causing bustages in /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h CLOSED TREE

Comment 19

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

One of my changes meant to be in D49976 ended up in D49980, and I was only landing D49976 but I'd tested with the full stack, so it went awry. Apologies.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/165ae98171b3
ExecutionReadyPromise should not be exclusive. r=perry

Comment 21

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

After feedback from :bholley and some introspection I'm abandoning the patches related to the conversion of "GenericPromise" to "GenericExclusivePromise". I think it probably makes sense for us to consider a combination of:

• Using explicit types/typedefs that are less homogeneous than Generic(Exclusive)Promise and use a naming convention that avoids implying JS Promise semantics.
• Just using direct lambda callbacks more and/or explicit callback interfaces. Since most of our uses turn out to be fine with exclusive promises and we frequently only have 1 or 2 threads in-play (versus MozPromises' media-motivated creation that can potentially have a ton of threads and the AbstractThread affinity concepts), this could potentially be beneficial overall.

I'm going to put this on my brain's back-burner though because I think various searchfox things stewing will address most of my concerns.

Comment 22

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/165ae98171b3

Comment 23

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

Comment on attachment 9103075 [details]
Bug 1583859 - ExecutionReadyPromise should not be exclusive. r=perry

Beta/Release Uplift Approval Request

• User impact if declined: MOZ_DIAGNOSTIC_ASSERT crashes on beta. The crashes won't happen on release, just beta.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This changes a low-level async abstraction from the "assert if used by more than 1 caller" mode to the "I expect to be called by multiple callers" mode. This is type-checked and otherwise doesn't change application logic beyond not crashing.
• String changes made/needed:

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9103075 [details]
Bug 1583859 - ExecutionReadyPromise should not be exclusive. r=perry

Fix for a low volume crash on beta, we don't have enough volume on nightly to know if it is effective so let's land it early in beta and see theeffect. Uplift approved for 71 beta 5, thanks.

Comment 25

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/80b49821b354

Comment 26

[Marcia Knous [:marcia]]

Adding another signature spotted in crash stats.

Comment 27

[Randell Jesup [:jesup] (needinfo me)]

I just got this (similar case, during a slack refresh) on a Nightly 11/11 build, so we're still hitting this (or something very closely related). I got the new signature Marcia added a couple of weeks ago.
Seems to be mac only

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

SHould we reopen, or file a followup?

Comment 29

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

Can you provide a link to your crash on crash-stats? I'm not seeing any.