1412007 - HttpChannelChild::SendDivertComplete must always be executed after SendDivertOnStopRequest

Status: RESOLVED INVALID

(Core :: Networking: HTTP, enhancement, P2)

Description

[Andrea Marchesini [:baku]]

The current code is fragile and it depends of the ordering of runnables in the event loop. Working on bug 1371699, my patches add an extra runnable and this logic breaks in a SW test: dom/workers/test/serviceworkers/browser_download.js

With this bug and, I want to enforce the ordering the calling.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: sw.patch

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8922367 [details] [diff] [review]
sw.patch

If you want me to review a patch like this then:
- white a good explanation to bugzilla what the problem was and how and why you are fixing the way you do
- add good comments to the code (mainly: why the state has to be atomic and why with that memory ordering?  what is each of the states meaning and how do they transit and how they influences the code paths?)

r- for lack of comments.

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> - white

write :D

Comment 4

[Andrea Marchesini [:baku]]

Attachment: sw.patch

Here a new patch with comments.

The reason why this is needed, is because in HttpChannelChild, there is nothing that guarantees SendDivertComplete to run after SendDivertOnStopRequest. 

In bug 1371699, I'm making any non-blocking non-async inputStream, able to used without having nsIStreamTransportService involved. This is done using a wrapper inputstream called NonBlockingAsyncInputStream. More can be read in bug 1371699.

My patches dispatch runnables differently, compared with the current setup based on a pipe (reading data on a different thread). What I see is that, intermittently, SendDivertOnStopRequest is called after SendDivertComplete. This should not happen. This patch enforces the correct ordering of the execution of these 2 methods.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8922721 [details] [diff] [review]
sw.patch

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

Thanks for the comments!  It really make it way more simple to follow the patch.  I would be interested why your patches are causing this switch, tho.  There could be a larger problem either here or in your patches when this happens.  Have you some data on it or can you provide it?

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ -615,5 @@
>    DoOnStartRequest(this, mListenerContext);
>  }
>  
> -namespace {
> -

any particular reason to expose this class?

Comment 6

[Andrea Marchesini [:baku]]

> happens.  Have you some data on it or can you provide it?

I asked bkelly and valentin. Both told me that there is nothing guaranteeing the current ordering.
With my patches I'm trying to avoid the using of a I/O thread for non-blocking and non-async inputStream and this changes the kind of Runnables we use.

I wound if we have intermittent failures related to this issue. Definitely we have many related to ServiceWorkers. 

> any particular reason to expose this class?

because SyntheticDiversionListener needs to call MaybeSendDivertComplete(). We can make it public, or make SyntheticDiversionListener a class friend.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35fc92e75cf7
HttpChannelChild::SendDivertComplete must always be executed after SendDivertOnStopRequest, r=mayhemer

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Andrea Marchesini [:baku] from comment #6)
> > happens.  Have you some data on it or can you provide it?
> 
> I asked bkelly and valentin. Both told me that there is nothing guaranteeing
> the current ordering.
> With my patches I'm trying to avoid the using of a I/O thread for
> non-blocking and non-async inputStream and this changes the kind of
> Runnables we use.
> 
> I wound if we have intermittent failures related to this issue. Definitely
> we have many related to ServiceWorkers. 

Thanks, this is a good rational.

> 
> > any particular reason to expose this class?
> 
> because SyntheticDiversionListener needs to call MaybeSendDivertComplete().
> We can make it public, or make SyntheticDiversionListener a class friend.

or nested (too late, you've landed).  I'm not happy to expose it from the module, but it's not critical.

Comment 9

[Andreea Pavel [:apavel]]

Backed out for failing broswer-chrome browser_bug676619.js

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=141694810&repo=mozilla-inbound&lineNumber=2949

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/58e3cc92a9e49a92ce07affb1c427df354bd913a

Comment 10

[Andrea Marchesini [:baku]]

This patch is actually not needed anymore.