1334465 - [e10s] Crash in IPCError-browser | PHttpChannel::Msg_SetPriority Route error: message sent to unknown actor ID

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-91708d65-f0f7-4262-8302-320302170127.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	ntdll.dll 	KiFastSystemCallRet 	
1 	ntdll.dll 	NtWaitForMultipleObjects 	
2 	kernel32.dll 	CreateFileMappingA 	
3 	user32.dll 	RealMsgWaitForMultipleObjectsEx 	
4 	xul.dll 	mozilla::widget::WinUtils::WaitForMessage(unsigned long) 	widget/windows/WinUtils.cpp:792
5 	xul.dll 	nsAppShell::ProcessNextNativeEvent(bool) 	widget/windows/nsAppShell.cpp:382
6 	xul.dll 	nsBaseAppShell::DoProcessNextNativeEvent(bool) 	widget/nsBaseAppShell.cpp:138
7 	xul.dll 	nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool) 	widget/nsBaseAppShell.cpp:289
8 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1189
9 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp:361
10 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:124
11 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:301
12 	xul.dll 	_SEH_epilog4

crash reports in the content process with this signature are regressing in firefox 52 builds and later.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

FWIW, most (but not all) reports are coming from WinXP. The stacks are all over the place too. I don't think Networking is the right component, but I have no clue what the right one should be either. Any thoughts, Jim?

Comment 2

[Jim Mathies [:jimm]]

Network looks right, this is an async ipdl message that ends up getting delivered after the receiver gets torn down. The stacks are going to be all over the place.

http://searchfox.org/mozilla-central/source/netwerk/protocol/http/PHttpChannel.ipdl#180

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Jason, can you please help find an owner for this?

Comment 4

[Dragana Damjanovic [:dragana]]

Valentin can you look into this? You have repair some of crashes like this.

Looking into the history, the oldest build id is 20161102030205.

Your patch for bug 1294719 landed 5 days before 2.Nov. Bug 1294719 should have repaired such crashes :)

Comment 5

[Valentin Gosu [:valentin] (he/him)]

This i

Comment 6

[Valentin Gosu [:valentin] (he/him)]

This is very strange, since calls to SendSetPriority are guarded in both the parent and child, and bug 1294719 should have fixed any race condition that may occur when deleting the channel. I'll investigate more.

Comment 7

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Too late for a fix for 53. Looks like most of the crashes have now moved to ESR52.

Comment 8

[Firefox Bug Husbandry Bot]

Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 9

[Ryan VanderMeulen [:RyanVM]]

433 crashes on ESR52 in the last week. Is there something actionable we can do in this bug?

Comment 10

[Valentin Gosu [:valentin] (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> 433 crashes on ESR52 in the last week. Is there something actionable we can
> do in this bug?

I think I figured this out. It turns out HttpChannelParent::mIPCClosed isn't atomic. This turns out to be a problem, as DoSendSetPriority sometimes gets called off-main-thread.
Good news is this patch is very easy to uplift to ESR, and risk free.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1334465 - Set mIPCClosed to true before calling SendDeleteSelf in order to avoid race

Review commit: https://reviewboard.mozilla.org/r/219038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219038/

Comment 12

[Daniel Stenberg [:bagder]]

Comment on attachment 8949700 [details]
Bug 1334465 - Set mIPCClosed to true before calling SendDeleteSelf in order to avoid race

https://reviewboard.mozilla.org/r/219038/#review224944

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd315ae86709
Make HttpChannelParent::mIPCClosed atomic r=bagder

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8949700 [details]
Bug 1334465 - Set mIPCClosed to true before calling SendDeleteSelf in order to avoid race

Approval Request Comment

[Feature/Bug causing the regression]:
unknown. Bug was filed in Firefox 52.

[User impact if declined]:
The content process crashes, with most of the crashes being in esr 52.

[Fix Landed on Version]:
Firefox 60

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Just landed.

[Needs manual test from QE? If yes, steps to reproduce]:
No. This is almost impossible to manually reproduce, especially on the latest versions.
 
[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No. It just changes a variable to be atomic in order to avoid data races.

[Why is the change risky/not risky?]:
It is a very simple change, unlikely to cause unintended behaviour.

[String changes made/needed]:
None.

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/bd315ae86709

Comment 16

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8949700 [details]
Bug 1334465 - Set mIPCClosed to true before calling SendDeleteSelf in order to avoid race

Canceling uplift as a followup patch is needed. (making the variable atomic doesn't completely solve the race)

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8949700 [details]
Bug 1334465 - Set mIPCClosed to true before calling SendDeleteSelf in order to avoid race

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219038/diff/1-2/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ebdab0e3328
Set mIPCClosed to true before calling SendDeleteSelf in order to avoid race r=bagder

Comment 19

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/4ebdab0e3328

Comment 20

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8949700 [details]
Bug 1334465 - Set mIPCClosed to true before calling SendDeleteSelf in order to avoid race

Approval Request Comment

[Feature/Bug causing the regression]:
Bug 1274886. Failures started in Firefox 52.

[User impact if declined]:
The content process crashes, with most of the crashes being in esr 52.

[Fix Landed on Version]:
Firefox 60

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Just landed.

[Needs manual test from QE? If yes, steps to reproduce]:
No. This is almost impossible to manually reproduce, especially on the latest versions.
 
[List of other uplifts needed for the feature/fix]:
https://hg.mozilla.org/mozilla-central/rev/bd315ae86709 (also landed in this bug)

[Is the change risky?]:
No. It just makes a variable atomic and makes sure to set it before sending the "supposed to be last" IPC message.

[Why is the change risky/not risky?]:
It is a very simple change, unlikely to cause unintended behaviour.

[String changes made/needed]:
None.

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/e78a36e944d2
Backed out changeset 4ebdab0e3328 for frequently failing marionette\test_refresh_firefox.py a=backout

Comment 22

[Arthur Iakab [arthur_iakab]]

Backed out changeset 4ebdab0e3328 (bug 1334465) for frequently failing marionette\test_refresh_firefox.py a=backout
Link to the log of failurehttps://treeherder.mozilla.org/logviewer.html#?job_id=161636850&repo=autoland&lineNumber=48779
Backou revision e78a36e944d2
Failing push https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bd315ae86709c3459a3dbf0778022ff3b1908723&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=success&filter-searchStr=marionette

Comment 23

[Valentin Gosu [:valentin] (he/him)]

(In reply to Arthur Iakab [arthur_iakab] from comment #22)
> Backed out changeset 4ebdab0e3328 (bug 1334465) for frequently failing
> marionette\test_refresh_firefox.py a=backout

That's really really odd. That patch really shouldn't be causing such issues.
From what I can tell in bug 1425323 failures started a few weeks ago, but for some reason they started to spike with my patch. I'll try to figure out why.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Valentin. Perhaps with this spike we can figure out what's actually causing it. If you want to run this test execute the following command and get all the log info:

> mach marionette test -vv --gecko-log - %path_to_test_file%

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4ad53516e0
Set mIPCClosed to true before calling SendDeleteSelf in order to avoid race r=bagder

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Relanded as https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4ad53516e01095be35542fd979c9e16d6e6b16 because the backout didn't fix the issue.

See https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=cb9b9f2707a33a1a85d5c2e74e2cd75a8afa337e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=pending&filter-resultStatus=running

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out again. The previous backout only reverted the follow-up fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f49cf3d147dee2eb1fd2b726cfbae6d01b719d55

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/bad527a1b855
Set mIPCClosed to true before calling SendDeleteSelf in order to avoid race r=bagder a=Aryx
https://hg.mozilla.org/mozilla-central/rev/db42a5fc7587
Backed out 2 changesets for frequently failing marionette\test_refresh_firefox.py a=Aryx

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Relanding: https://hg.mozilla.org/mozilla-central/rev/bad527a1b855
Backout of both patches: https://hg.mozilla.org/mozilla-central/rev/db42a5fc7587

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

There are still 2 of those failures on the background push, so far. https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=db42a5fc758778ce332dac0c9026ced63ab42a95&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 31

[Valentin Gosu [:valentin] (he/him)]

I see a bunch of failures since the full backout. I take it that confirms that we can land it back again?

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8949700 [details]
Bug 1334465 - Set mIPCClosed to true before calling SendDeleteSelf in order to avoid race

content crash fix, Beta59+, ESR52+

Comment 33

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/60c4b07c49b5
https://hg.mozilla.org/releases/mozilla-beta/rev/6ad52cf5551f

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48b9f6671588
Make HttpChannelParent::mIPCClosed atomic r=bagder
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfc9afe91609
Set mIPCClosed to true before calling SendDeleteSelf in order to avoid race r=bagder

Comment 35

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/48b9f6671588
https://hg.mozilla.org/mozilla-central/rev/cfc9afe91609

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/f95c5b881442
https://hg.mozilla.org/releases/mozilla-esr52/rev/8a4265c8fb41