742935 - Some pipelining patches has not been landed correctly

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Honza Bambas (:mayhemer)]

See bug 742827.  I've found out during review of one of the pipelining crash bugs that by accident older versions (r-'ed, btw) of the patches could have been landed instead of the latest ones.

We should go through those patches one by one and check all had been landed as expected.

Here is the push log:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=ee6328d11bfb

Comment 1

[Patrick McManus [:mcmanus]]

This audit was worth doing. 3 more of the patches were slightly incorrect. None of them appear to carry any serious changes in correctness or functionality and therefore are unlikely to be the source of nightly problems. I'll attach 3 patches here to bring them up to their intended state - I don't see a reason to track them all as separable bugs.

I'm really annoyed that I can't figure out how this happened.

should be v1 is v0
summary:     bug 717759 - negative pipeline feedback on img decode problem r=bz r=jdrew r=honzab

should be v2.. is v1
summary:     bug 717350 doom cache entry on last-modified mismatch r=honzab

should be v10.. is v9.
summary:     bug 603512 - large objects block pipelines r=honzab

The rest of the patches were confirmed to be ok as best as I can tell.. generally I did this by looking at the review comments that generated the last patch (or were given as a conditional ok to the last patch) and tested at least one aspect of that against m-c to make sure the right code was there. my notes on each:

should be v9.. confirmed that pipelinereadtimeout is set in ms as part
of ctor list which was v8 review comment
summary:     bug 603514 - http stalled read detection r=honzab

should be v4.. confirmed that typo of "thist" from v3 had been corrected
summary:     bug 665885 respect keepalive: "max=" attribute r=honzab

should be v0 plus review comment.. confirmed that i from comment 2 is signed
summary:     bug 737155 - httpconnectionmgr::processPendingQ allow more than 1 with pipeline r=honzab

should be v17.. confirmed by looking for k1200 constant
summary:     bug 599164 pipeline with type and state r=honzab

should be v7 confirmed by looking for check of return value of ns_newuri()
summary:     bug 597684 Implement HTTP Assoc-req and Banned Pipelines on nsHttpConnectionInfo r=honzab

confirmed v13 + requested changes such as IsProxyConnectInProgress name
summary:     bug 447866 http pipelining is bursty r=honzab

ok - v0 was only one
summary:     bug 447866 - patch pre.1 for empty nsHttpPipeline::TakeSubTransactions r=honzab

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: fixup 603512 v0

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: fix 717350 v0

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: fixup 717759 v0

Comment 5

[Honza Bambas (:mayhemer)]

Here should be static const I think: http://hg.mozilla.org/mozilla-central/annotate/ad90865125a2/netwerk/protocol/http/nsHttpConnection.cpp#l1338.

I also found out that bug 603514 has landed finally wrong.  Seems like a variant of v7 patch from that bug has actually landed.  There are cross changes with patch from bug 728113 that makes it harder to figure out correctly, but some parts are missing.  Please check that too.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 612786 [details] [diff] [review]
fixup 603512 v0

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

r=honzab

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 612787 [details] [diff] [review]
fix 717350 v0

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

Can you please just `hg mv` the test file to track changes?

r=honzab with that.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 612788 [details] [diff] [review]
fixup 717759 v0

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

r=honzab

BTW, do we have some telemetry for this?  It would be interesting to know how often images are broken.  Probably before we enable pipelining by default on.

Comment 9

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf132b7a5a9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/10f864a9b5c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc8f2d891d8f

Comment 10

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> Here should be static const I think:
> http://hg.mozilla.org/mozilla-central/annotate/ad90865125a2/netwerk/protocol/
> http/nsHttpConnection.cpp#l1338.
> 
> I also found out that bug 603514 has landed finally wrong.  Seems like a
> variant of v7 patch from that bug has actually landed.  There are cross
> changes with patch from bug 728113 that makes it harder to figure out
> correctly, but some parts are missing.  Please check that too.

this one is a bit more complicated (the others I could do pretty much automatically), so I have split this into 743747. The complications stems from the fact that much of it was merged for spdy's use and the bit rot was pretty extensive.

my approach was just to go through the last patch by hand and make sure each piece was right.

the primary issue was a couple of things that should have been in prefs that were left hardcoded. The patch also introduced a bool for honoring the timeout value and since the landing I changed that code to allow '0' to mean the same thing. I resolved the conflict in favor of the bool.

Comment 11

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/dc8f2d891d8f
https://hg.mozilla.org/mozilla-central/rev/10f864a9b5c5
https://hg.mozilla.org/mozilla-central/rev/bf132b7a5a9d