1180886 - crash in mozilla::ipc::SerializeInputStream(nsIInputStream*, mozilla::ipc::InputStreamParams&, nsTArray<T>&)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jim Mathies [:jimm]]

#5 top e10s crasher on aurora, looks netwerk related.

reports:
https://crash-stats.mozilla.com/signature/?product=Firefox&dom_ipc_enabled=%21__null__&version=41.0a2&signature=mozilla%3A%3Aipc%3A%3ASerializeInputStream%28nsIInputStream%2A%2C+mozilla%3A%3Aipc%3A%3AInputStreamParams%26%2C+nsTArray%3CT%3E%26%29

individual crash report:
https://crash-stats.mozilla.com/report/index/8a258a8a-f2fe-4782-bbee-ddfcf2150704

Comment 1

[Josh Matthews [:jdm]]

Do we have any URLs for these? This might be covered by bug 1167730.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

The few stacks I looked at all had FetchEvent in them.  My guess is that someone is uploading a body and did Request.clone() or something.  (Although I guess other stacks don't have FetchEvent.)

I have plans to implement bug 1093357, but just need to figure out how to prioritize that vs other service worker bugs.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Of the most recent crashes, all of them seem to go through either FetchEvent or ResetInterception (the only caller of which is FetchEventRunnable). bkelley thought that we disabled onfetch (please correct me if I'm wrong), but downloading Developer Edition and launching it with an empty profile gives me both dom.serviceWorkers.interception.enabled and dom.serviceWorkers.enabled as true by default. This suggests doing something like disabling fetch if e10s is enabled on Aurora.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Untested patch

This might help. I'm going to push to try to see if this breaks anything. bkelley, does this seem reasonable to you?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8645255 [details] [diff] [review]
Untested patch

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

Uh.  This would basically just turn off fetch event in e10s regardless of the dom.serviceWorkers.interception.enabled pref.

We should just disable the pref in aurora.  Make this #if defined(NIGHTLY_BUILD):

  https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#1949

Ehsan, do you agree? (I was not aware we enabled fetch event for aurora again...)

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8645255 [details] [diff] [review]
Untested patch

I think Ehsan is out this week, so I'll just be more decisive.

Blake, please just make that pref NIGHTLY_BUILD only.  I'll modify this bug to be leave-open and have it block us shipping service workers v1.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: As requested

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

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8645870 [details] [diff] [review]
As requested

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

Thanks for doing this Blake.  r=me with comments addressed.

::: browser/app/profile/firefox.js
@@ +1971,1 @@
>  pref("dom.serviceWorkers.interception.enabled", true);

nit: Can you move this out of the #ifndef RELEASE_BUILD?  Seems unnecessarily complex to nest the conditions.

Also, it seems we need to do this for android in mobile.js.  See:

  https://dxr.mozilla.org/mozilla-central/source/mobile/android/app/mobile.js#932

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Full patch

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

The "full patch" causes tests to fail. I'll hopefully have time to patch them up tomorrow.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Its likely we are fixing these tonight because they broke on the merge to beta.  See bug 1192996, etc.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Actually, I'm going to be just disabling the tests in beta instead of adding the prefs.  What you need, though, is to just uplift this file to aurora:

  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/meta/service-workers/service-worker/__dir__.ini?case=true&from=testing%2Fweb-platform%2Fmozilla%2Fmeta%2Fservice-workers%2Fservice-worker%2F__dir__.ini

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

I think this should now be fixed by a merge from last week that had a patch from Nikhil that did exactly this. Looking at the reports from comment 0, I think I'm right, so I'm marking this FIXED. Jim, could you double-check?

Comment 15

[Jim Mathies [:jimm]]

yep, looks fixed. I don't see anything in 42.0a2.

Comment 16

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.