Closed Bug 1348675 Opened 3 years ago Closed 3 years ago

Disable pipelining on android

Categories

(Core :: Networking: HTTP, defect)

55 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- fixed
firefox55 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

(Whiteboard: [necko-active])

Attachments

(2 files)

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: nobody → dd.mozilla
Blocks: 1347856
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
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+
Keywords: checkin-needed
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?
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.
https://hg.mozilla.org/mozilla-central/rev/7c13e966130b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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)
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)
This is a patch for beta.
Flags: needinfo?(dd.mozilla)
Attachment #8851036 - Flags: review?(mcmanus)
Attachment #8851036 - Flags: review?(mcmanus) → review+
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+
You need to log in before you can comment on or make changes to this bug.