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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

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

Comment 1

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=ff67b4eb7c5f

Comment 2

[Patrick McManus [:mcmanus]]

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.

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: commit ff91cbbed5fe0b82306c1203d008e17808f8c443

Author: Patrick McManus <[e-mail]>
    bug 1076129 generate event on socket transport cancelation r=sworkman

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8498877 [details] [diff] [review]
commit ff91cbbed5fe0b82306c1203d008e17808f8c443

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

Looks good. r=me.

Comment 5

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1babd65ebec7

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Something in this push was causing frequent xpcshell crashes. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1adbc85fcfce

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2725236&repo=mozilla-inbound

Comment 7

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77c1b79c5bec

Comment 8

[Phil Ringnalda (:philor)]

(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

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

So, uh, what the hell were you thinking? Do you believe that this kind of behaviour is actually acceptable?

Comment 10

[Patrick McManus [: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.

Comment 11

[Patrick McManus [:mcmanus]]

Attachment: generate event on socket transport cancelation

Comment 12

[Patrick McManus [:mcmanus]]

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.

Comment 13

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/896ed43b6b61

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/896ed43b6b61