Last Comment Bug 742935 - Some pipelining patches has not been landed correctly
: Some pipelining patches has not been landed correctly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla14
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on: 742827 743747
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-05 13:55 PDT by Honza Bambas (:mayhemer)
Modified: 2012-04-10 08:28 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fixup 603512 v0 (8.99 KB, patch)
2012-04-05 19:50 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review
fix 717350 v0 (14.61 KB, patch)
2012-04-05 19:51 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review
fixup 717759 v0 (6.54 KB, patch)
2012-04-05 19:52 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review

Description Honza Bambas (:mayhemer) 2012-04-05 13:55:37 PDT
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] 2012-04-05 19:41:24 PDT
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] 2012-04-05 19:50:11 PDT
Created attachment 612786 [details] [diff] [review]
fixup 603512 v0
Comment 3 Patrick McManus [:mcmanus] 2012-04-05 19:51:22 PDT
Created attachment 612787 [details] [diff] [review]
fix 717350 v0
Comment 4 Patrick McManus [:mcmanus] 2012-04-05 19:52:15 PDT
Created attachment 612788 [details] [diff] [review]
fixup 717759 v0
Comment 5 Honza Bambas (:mayhemer) 2012-04-06 08:53:16 PDT
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) 2012-04-06 08:56:55 PDT
Comment on attachment 612786 [details] [diff] [review]
fixup 603512 v0

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

r=honzab
Comment 7 Honza Bambas (:mayhemer) 2012-04-06 09:00:41 PDT
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) 2012-04-06 09:04:06 PDT
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 10 Patrick McManus [:mcmanus] 2012-04-09 11:32:28 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.