1359951 - Crash in mozalloc_abort | NS_DebugBreak | mozilla::ipc::FatalError | mozilla::net::PNeckoChild::SendPWebSocketConstructor

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-f7642eda-91c8-4cb0-86ed-3b4502170328.
=============================================================

Seen while looking at nightly crash stats - feel feel to move to correct component - crashes started on Nightly using 20170308030207 - http://bit.ly/2oMCOqg 

No useful correlations or comments.

Comment 1

[alex_mayorga]

¡Hola!

Yup! This bite me on Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0 ID:20170426030329 CSet: 0f5ba06c4c5959030a05cb852656d854065e2226 in the form of bp-9c6752f7-a4b6-470d-b67c-992f10170427 

Subscribing...

¡Gracias!
Alex

Comment 2

[u408661]

Shian-Yow - do we have anyone in Taipei who has e10s and/or websockets knowledge that could take a look at this? I've already bugged Michal on a few things lately (my usual websockets go-to), and don't want to overload him :)

Comment 3

[Shian-Yow Wu [:swu]]

(In reply to Nicholas Hurley [:nwgh][:hurley] (also [e-mail]) from comment #2)
> Shian-Yow - do we have anyone in Taipei who has e10s and/or websockets
> knowledge that could take a look at this? I've already bugged Michal on a
> few things lately (my usual websockets go-to), and don't want to overload
> him :)

I would like Junior to take a look at this. :)

Comment 4

[Junior [inactive]]

It has the pattern in Bug 1332054, failed in constructor in a shutdown progress.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Junior[:junior] from comment #4)
> It has the pattern in Bug 1332054, failed in constructor in a shutdown
> progress.

Trying to create IPDL during shutdown phase doesn't seems something we should do. Is it triggered by test case or something else?

Comment 6

[Junior [inactive]]

I did a quick investigation that we don't have a intermittent for this before.

Comment 4 is judged by
1. It's triggered from child process
2. Most of them happened in ShutdownProgress:profile-before-change

Comment 7

[Junior [inactive]]

FWIW, It's interested that the uptime ranges are < 1min

Comment 8

[Junior [inactive]]

Attachment: websocket-crash - v1

The FatalError occurs in 
PNeckoChild::SendPWebSocketConstructor() ->
(GetIPCChannel())->Send(msg__);

And all the cases are in shutdown progress.

Similar to Bug 1277582, we need to have a bail-out.

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

Comment 9

[u408661]

Comment on attachment 8863697 [details] [diff] [review]
websocket-crash - v1

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

Nit on the commit message: use r=nwgh (or r=hurley, if you're old-school). "nick" is a bit too ambiguous :)

::: netwerk/protocol/websocket/WebSocketChannelChild.cpp
@@ +556,5 @@
>  
> +  ContentChild* cc = static_cast<ContentChild*>(gNeckoChild->Manager());
> +  if (cc->IsShuttingDown()) {
> +    return NS_ERROR_FAILURE;
> +  }

So like in bug 1277582, this should probably be moved above the AddIPDLReference() call further up in the method. Otherwise, this is good to go.

Comment 10

[u408661]

Also, :billm mentioned over in bug 1277582 that there needs to be a way to prevent this from happening globally. Bill, did that bug ever get filed?

Comment 11

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

No, although bug 1332054 would probably be that bug. I need to get back to that at some point.

Comment 12

[Junior [inactive]]

Attachment: websocket-crash - v2

Thanks for your prompt feedback!

Comment 13

[u408661]

Comment on attachment 8863967 [details] [diff] [review]
websocket-crash - v2

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

LGTM, thanks!

Comment 14

[Junior [inactive]]

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

Comment 15

[Junior [inactive]]

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/816f2cd29ba6
Ensure that the WebSocket not sending constructor when shutting down, r=nwgh

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/816f2cd29ba6

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. While ESR52 also appears to be affected, the crash rate is miniscule, so I'm setting the status to wontfix. Feel free to change it to affected and nominate the patch for approval if you feel strongly that it warrants consideration for that branch as well.

Comment 19

[Junior [inactive]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Please request Beta approval on this when you get a chance. While ESR52 also
> appears to be affected, the crash rate is miniscule, so I'm setting the
> status to wontfix. Feel free to change it to affected and nominate the patch
> for approval if you feel strongly that it warrants consideration for that
> branch as well.

It's a shut-down crash. IMO it's a wontfix to ESR

Comment 20

[Junior [inactive]]

Comment on attachment 8863967 [details] [diff] [review]
websocket-crash - v2

Approval Request Comment
[Feature/Bug causing the regression]: No bug
[User impact if declined]: Have shut-down crashes
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: Not risky
[Why is the change risky/not risky?]: It's a shut-down check and we already have some similar fix
[String changes made/needed]: No

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8863967 [details] [diff] [review]
websocket-crash - v2

Fix a crash. Beta54+. Should be in 54 beta 7.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/40c0c72fd4ef