HttpChannelChild::SendDivertComplete must always be executed after SendDivertOnStopRequest

RESOLVED INVALID

Status

()

enhancement
P2
normal
RESOLVED INVALID
2 years ago
Last year

People

(Reporter: baku, Assigned: baku)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch sw.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8922367 - Flags: review?(honzab.moz)
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.
Attachment #8922367 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #2)
> - white

write :D
Posted patch sw.patchSplinter Review
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.
Attachment #8922367 - Attachment is obsolete: true
Attachment #8922721 - Flags: review?(honzab.moz)
Priority: -- → P2
Whiteboard: [necko-triaged]
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?
Attachment #8922721 - Flags: review?(honzab.moz) → review+
> 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.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35fc92e75cf7
HttpChannelChild::SendDivertComplete must always be executed after SendDivertOnStopRequest, r=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.
This patch is actually not needed anymore.
Status: NEW → RESOLVED
Closed: Last year
Flags: needinfo?(amarchesini)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.