1136969 - Assert that we're not sending OnStartRequest more than once

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Seth Fowler [:seth] [:s2h]]

We've seen multiple crashes and assertion failures in ImageLib lately that suggest that OnStartRequest may be getting delivered more than once. Examples are bug 1130607 and bug 1135272.

I'm adding checks inside ImageLib for this, but if this theory is correct they're just working around the real issue. I think we'll catch it faster if we assert for this everywhere, not just in ImageLib.

It'd be great if we assert that we're not sending duplicate OnStartRequest calls in Necko itself.

Comment 1

[Patrick McManus [:mcmanus]]

this is just a drive-by kibitzing comment.

make sure that what you are observing in imageLib isn't the result of a redirect somehow. A single channel should only onStartRequest() you once, but if a redirect was involved you would certainly get multiple channels and onStartRequest()s for the same resource load.

I'm certainly fine with the asserts in the channel code :)

Comment 2

[Seth Fowler [:seth] [:s2h]]

I'm not sure that the ImageLib code has been designed consciously with that in mind, unfortunately. I at least had assumed that we only got OnStartRequest after all redirects were complete. I'll audit the code with that in mind.

However, at least one of those crashes seems to involve a case where we get OnStartRequest again after OnDataAvailable is delivered. I'm pretty sure that shouldn't be possible except in the multipart/x-mixed-replace case (which wasn't what was happening).

Comment 3

[Jason Duell]

Dragana, can you take this?  I think just adding a mOnStartRequest variable and checking it before we call OnStartRequest is the right fix here.

Comment 4

[Dragana Damjanovic [:dragana]]

Attachment: bug_1136969_v1.patch

Comment 5

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c48331640b7

Comment 6

[Seth Fowler [:seth] [:s2h]]

Jason, it looks like this is waiting on review from you. It'd be awesome if you could look at this soon. This is a big concern for folks working on image and graphics-related crashes.

Comment 7

[Seth Fowler [:seth] [:s2h]]

It'd be great to get this landed. Evidence keeps mounting that we may be sending OnStartRequest more than once, though I've never been able to get it to happen locally.

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 8569816 [details] [diff] [review]
bug_1136969_v1.patch

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

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +414,5 @@
>    nsCOMPtr<nsIPrincipal>            mPrincipal;
>  
>    bool                              mForcePending;
>    nsCOMPtr<nsIURI>                  mTopWindowURI;
> +  mozilla::Atomic<bool>             mOnStartRequestCalled;

why the atomic? Is non-main thread involved somehow?

Comment 9

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #8)
> Comment on attachment 8569816 [details] [diff] [review]
> bug_1136969_v1.patch
> 
> Review of attachment 8569816 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/HttpBaseChannel.h
> @@ +414,5 @@
> >    nsCOMPtr<nsIPrincipal>            mPrincipal;
> >  
> >    bool                              mForcePending;
> >    nsCOMPtr<nsIURI>                  mTopWindowURI;
> > +  mozilla::Atomic<bool>             mOnStartRequestCalled;
> 
> why the atomic? Is non-main thread involved somehow?


No of course it is should be all on the main thread.

Comment 10

[Dragana Damjanovic [:dragana]]

Attachment: bug_1136969_v2.patch

Comment 11

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e719bca40ab6

Comment 12

[Seth Fowler [:seth] [:s2h]]

Thanks so much for getting this finished! Really appreciated!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5ca2d2c7c0f

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d5ca2d2c7c0f