Closed Bug 1076129 Opened 5 years ago Closed 5 years ago

services/sync/test_syncscheduler.js and test_errorhandler_sync_checkServerError.js fail on os x withnetwork.http.connection-retry-timeout = 0

Categories

(Core :: Networking: HTTP, defect)

32 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

Attachments

(1 file, 1 obsolete file)

set network.http.connection-retry-timeout = 0 (or disable the code path with an early return in nshttpconnectionmgr::setupbacktimer())

run the xpcshell tests on os x.. and services/sync/{test_syncscheduler.js,test_errorhandler_sync_checkServerError.js}  fail for both debug and opt, both 10.6 and 10.8

other platforms are not impacted

I'm not really sure why
Blocks: 853423
The root problem here is the way 1918 speculative connects are canceled..

if an address resolves to 1918 (or in the case of these tests, localhost) then any speculative connects are aborted before the tcp syn - we do this to work around some lame home routers basically.

the socket transport marks them as having an error, in the usual way any other socket would do so. No event is generated.

but then they are left in the connection pool for some unsuspecting transaction to come along and pick up and try and use - and they get the error on a perfectly valid address.

There is also a problem that a speculative connect can be made non-speculative during MakeNewConnection(). i.e. instead of making a new connection an existing speculative one is attached to a transaction part way in progress. That's fine - but it needs to have the DISALLOW_1918 flag cleared so that the socket transport doesn't abort it.
Author: Patrick McManus <mcmanus@ducksong.com>
    bug 1076129 generate event on socket transport cancelation r=sworkman
Attachment #8498877 - Flags: review?(sworkman)
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Comment on attachment 8498877 [details] [diff] [review]
commit ff91cbbed5fe0b82306c1203d008e17808f8c443

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

Looks good. r=me.
Attachment #8498877 - Flags: review?(sworkman) → review+
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #6)
> Something in this push was causing frequent xpcshell crashes. Backed out.

Backed out again for the same thing as before in https://hg.mozilla.org/integration/mozilla-inbound/rev/759811d71d32
So, uh, what the hell were you thinking? Do you believe that this kind of behaviour is actually acceptable?
Flags: needinfo?(mcmanus)
(In reply to :Ms2ger from comment #9)
> So, uh, what the hell were you thinking?

I'd be happy to let you know what happened. Please remember we're on the same team and all trying to do the right thing - I'd appreciate a more civil tone of dialogue.

1] this bug was pushed as part of 3 csets covering two dependent bugs. 1003448 is the other.
2] When I pushed it I erred a bit in the mechanics - the csets were still in my hg queue, which normally a hook prevents but for reasons unknown it did not error out on me that time.
3] I really track this work under bug 1003448 (90%+ of the change is in that bug)
4] The backout wasn't mentioned in 1003448 and for whatever reason I did not notice comment 6 in this bug
5] When I noted the next day that 1003448 hadn't made it to mozilla-central and there was no activity in that bug, I checked and noted it missing from inbound.. I concluded wrongly that I had managed to make a new head back at #2 - I didn't realize it was backed out because it wasn't noted in the other bug.
6] So I repushed it.
7] obviously it was the same code with the same problem and it was backed out again.

> Do you believe that this kind of
> behaviour is actually acceptable?

hth. I'm moving on to fixing it.
Flags: needinfo?(mcmanus)
Attachment #8498877 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=4080bf80cb32

same as last patch but with a new null check. simple.

I needed to consider why that was null, but its perfectly normal for OnOutputStreamReady to change the half open socket to null when it associates it with the connection that will process the failure.
Attachment #8503142 - Flags: review?(sworkman) → review+
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/896ed43b6b61
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.