bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Disable pipelining on android

RESOLVED FIXED in Firefox 53

Status

()

Core
Networking: HTTP
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

55 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox53 fixed, firefox55 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(2 attachments)

(Assignee)

Description

a year ago
Pipelining is removed in FF54 - bug 1340655. We can disable it in 53. Pipelining causes some crashes like: bug 1347856.


(bug 1340655 missed to remove prefs for pipelining from firefox for android)
(Assignee)

Updated

a year ago
Assignee: nobody → dd.mozilla
Blocks: 1347856
Status: NEW → ASSIGNED
status-firefox53: --- → affected
Whiteboard: [necko-active]
(Assignee)

Comment 1

a year ago
Created attachment 8848889 [details] [diff] [review]
bug_1348675.patch
Attachment #8848889 - Flags: review?(mcmanus)
Comment on attachment 8848889 [details] [diff] [review]
bug_1348675.patch

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

thanks
Attachment #8848889 - Flags: review?(mcmanus) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Comment 3

a year ago
Comment on attachment 8848889 [details] [diff] [review]
bug_1348675.patch

Approval Request Comment
[Feature/Bug causing the regression]: http pipelining
[User impact if declined]: some bugs in the pipelining code cause crashes like bug 1347856. We removed the pipelining completely on firefox 54 and the pipelining is disabled on desktop for very long time. This patch will disable it on android as well that will reduce crashes like bug 1347856.
[Is this code covered by automated tests?]: it is only a pref change.
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]: no
[Why is the change risky/not risky?]: none. it is only a pref change that disabled the pipelining on android. The pipelining is disabled on desktop for very long time.
[String changes made/needed]: none
Attachment #8848889 - Flags: approval-mozilla-beta?

Comment 4

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c13e966130b
Remove the separate prefs for the pipelining on the android version of firefox. r=mcmanus
Keywords: checkin-needed
what's the level of impact on 53/54 for android? if possible I'd prefer for the 55 changes just to ride the trains. (the patch I r+'d is for 55).
where I said 55 I meant 54 - Its removed in 54.

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7c13e966130b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Marco, do you have any advice for searching for these crashes?
Flags: needinfo?(mcastelluccio)
Dragana, does this just disable prefs, or does it remove them completely? The patch looks like it's removing them (in which case, if we release without the prefs and something goes wrong, we wouldn't be able to fix it with a system add-on)

If this isn't for a top crash (maybe there are many crashes I'm not seeing...)  then I'd rather that this ride the train, as Patrick suggests.
Flags: needinfo?(dd.mozilla)
Attachment #8848889 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
I think this search should be pretty comprehensive:
https://crash-stats.mozilla.com/search/?proto_signature=~nsHttpPipeline&product=FennecAndroid&date=%3E%3D2017-03-16T11%3A01%3A00.000Z&date=%3C2017-03-23T11%3A01%3A00.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

There are ~1750 crashes during the past week with a `nsHttpPipeline` function in the stack trace (a subset of these crashes might be due to something else, but at least the top 4 look definitely related and they account to ~1600 crashes).
Flags: needinfo?(mcastelluccio)
(Assignee)

Comment 12

a year ago
Thanks Marco!

This pref exist in modules/libpref/init/all.js as well. One is the general one and the other place is only for FennecAndroid. So deleting it from mobile/android/app/mobile.js will make the one from modules/libpref/init/all.js be used.

Pipelining is turned off on desktop for very very long time and FF54 does not have pipelining any more the whole code is removed.

I could only change the pref value in mobile/android/app/mobile.js as well, if we want to make this change on beta.
Flags: needinfo?(dd.mozilla)
That's a lot of crashes. How about just disabling the pref for beta? 
Patrick, do you have an opinion here?
Flags: needinfo?(mcmanus)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #13)
> That's a lot of crashes. How about just disabling the pref for beta? 
> Patrick, do you have an opinion here?

you can - its probably been like that for a while tho; this is just about the risk of pushing something out faster. fwiw that's what dragana's patch does in effect, but I agree that it would be clearerto take one that just explicitly set it to false for beta.
Flags: needinfo?(mcmanus)
Dragana, want to rewrite the patch? Sorry, I just worry about removing something altogether this late in beta.
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 16

a year ago
Created attachment 8851036 [details] [diff] [review]
bug_1348675_beta.patch

This is a patch for beta.
Flags: needinfo?(dd.mozilla)
Attachment #8851036 - Flags: review?(mcmanus)
Attachment #8851036 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 17

a year ago
Comment on attachment 8851036 [details] [diff] [review]
bug_1348675_beta.patch

Approval Request Comment
[Feature/Bug causing the regression]: Pipelining. It exist for very long time.
[User impact if declined]: It cause some crashes like bug 1347856. The pipelining code is old and it is removed in ff 54. The pipelining is disabled on desktop for very long time. This patch disables it on android as well.
[Is this code covered by automated tests?]: 
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: none.
[Why is the change risky/not risky?]:  The patch changes a pref to false on android; the same pref is false on desktop for very long time.
[String changes made/needed]: none
Attachment #8851036 - Flags: approval-mozilla-beta?
Comment on attachment 8851036 [details] [diff] [review]
bug_1348675_beta.patch

Some crashes may be caused by this. Disable pref for beta. Beta53+.
Attachment #8851036 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 19

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/04e4966e5181
status-firefox53: affected → fixed
You need to log in before you can comment on or make changes to this bug.