1380255 - Make FetchDriver thread safe

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Dragana Damjanovic [:dragana]]

FetchDrive implements nsIThreadRetargetableStreamListener and declares NS_DECL_ISUPPORTS.
It should be NS_DECL_THREADSAFE_ISUPPORTS if it wants to be retargetable.

Comment 1

[Dragana Damjanovic [:dragana]]

I found that nsWebRequestListener and nsStreamListenerWrapper have the same problem. I have open bugs for them too. I do not know if the problem is going to be triggered at all for them (it will not be triggered if one of the listeners in a chain vetoes retargeting), but anyway it should be fixed.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Why is this only a problem in FF56?  Couldn't this have triggered in non-e10s in older releases?

Comment 3

[Dragana Damjanovic [:dragana]]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #2)
> Why is this only a problem in FF56?  Couldn't this have triggered in
> non-e10s in older releases?

I think it is connected with nsUknownDecoder somehow, which is retargetable only since 56. It was not triggered before 56 (or at least we do not know about it). Maybe I am wrong. Please change flags.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Ok, that makes sense.

Comment 5

[Dragana Damjanovic [:dragana]]

Ben, can you take this or do you know if there is someone who can take it?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Andrew, can you find someone to work this?  I am just starting to dig out of PTO email.

Comment 7

[Andrew Overholt [:overholt]]

Sorry for the delay here. I think Catalin is probably best positioned to take this when he's back from PTO. He's got some other stuff he's trying to land but can probably parallelize and focus on this 100% when that's finished.

Comment 8

[Dragana Damjanovic [:dragana]]

In bug 1379631, I have disabled OMT delivery for FetchDriver. Please turn it on again in this bug. That is also a work around for this bug on 56.

Comment 9

[Dragana Damjanovic [:dragana]]

I am marking this unaffected for 56 as a work around landed in bug 1379631.

We can still uplift to 56 fix for this bug and turn on OMT if we want.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Given comment 8, can we remove the privacy bit here?  It doesn't seem like an active security problem.

Comment 11

[Cătălin Badea (:catalinb)]

Attachment: FetchDriver should have threadsafe refcounting.

Don't think there's anything else to be done, FetchDriver is already designed
to accept OnDataAvailable off the main thread.

Comment 12

[Cătălin Badea (:catalinb)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22a322b57f5000dcd4a28b52d836a6dbec97eaa6
Is the bot sleeping?

Landed this without sec-approval because there was a fix landed in firefox 56 (comment #9) and esr is marked as unaffected.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(In reply to Cătălin Badea (:catalinb) from comment #12)
> Is the bot sleeping?

Pulsebot doesn't have access to sec bugs.

Comment 14

[Cătălin Badea (:catalinb)]

Backed out due to test failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/999b0a7cc8dc

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=22a322b57f5000dcd4a28b52d836a6dbec97eaa6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

This shouldn't be a sec bug per comment 8.

Comment 16

[Andrew Overholt [:overholt]]

(In reply to Cătălin Badea (:catalinb) from comment #14)
> Backed out due to test failures:

Are there plans to re-land this?

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Since Catalin is busy with child array list I'm going to steal this.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

It seems that test_ext_webrequest_responseBody.html expects to attach the to the channel and receive OnDataAvailable() calls on the main thread.  The test just happens to use fetch() here, as far as I can tell.

Kris, is there some mechanism that webrequest uses to disable channel retargeting for other code?  Maybe we are missing something in fetch()?  Or is this possibly broken for all webrequest uses where channels are retargeted to other threads?

Comment 19

[Kris Maglione [:kmag]]

(In reply to Ben Kelly [:bkelly] from comment #18)
> It seems that test_ext_webrequest_responseBody.html expects to attach the to
> the channel and receive OnDataAvailable() calls on the main thread.  The
> test just happens to use fetch() here, as far as I can tell.
> 
> Kris, is there some mechanism that webrequest uses to disable channel
> retargeting for other code?  Maybe we are missing something in fetch()?  Or
> is this possibly broken for all webrequest uses where channels are
> retargeted to other threads?

It doesn't expect to receive those calls on the main thread. It generally prefers to receive data on the STS thread, but should work on any thread. The data always gets filtered through the main thread of the extension process, but the result always gets re-dispatched on whichever thread the last onDataAvailable call happens on.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Hmm, it seems webrequest tries to support retargeting:

https://searchfox.org/mozilla-central/source/toolkit/components/extensions/webrequest/StreamFilterParent.cpp#120
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/webrequest/ChannelWrapper.cpp#946

And in e10s mode it seems the StreamFilter IPC should execute on the main thread there.

Maybe this is more of a race between the stream filter and the fetch() response.text() resolving.

I'll debug tomorrow.  Sorry for the flag spam.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Actually, it looks like fetch() lost some data in there:

fetch() produced:
```
<!DOCTYPE html>\n    <html lang=\"en\">\n    <head>\n      <meta charset=\"UTF-8\">\n      <title></title>\n    </head>\n    <body>\nconsectetur adipiscing elit, <br>\nsed do eiusmod tempor incididunt ut labore et dolore magna aliqua. <br>\nUt enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. <br>\nDuis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. <br>\nExcepteur sint occaecat cupidatat non proident, <br>\nsunt in culpa qui officia deserunt mollit anim id est laborum.<br>\n\n    </body>\n    </html>\n
```

webrequest saw:
```
<!DOCTYPE html>\n    <html lang=\"en\">\n    <head>\n      <meta charset=\"UTF-8\">\n      <title></title>\n    </head>\n    <body>\nLorem ipsum dolor sit amet, <br>\nconsectetur adipiscing elit, <br>\nsed do eiusmod tempor incididunt ut labore et dolore magna aliqua. <br>\nUt enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. <br>\nDuis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. <br>\nExcepteur sint occaecat cupidatat non proident, <br>\nsunt in culpa qui officia deserunt mollit anim id est laborum.<br>\n\n    </body>\n    </html>\n
```

Comment 22

[Kris Maglione [:kmag]]

(In reply to Ben Kelly [:bkelly] from comment #21)
> Actually, it looks like fetch() lost some data in there:

Yeah, there's a bug for that that's on my todo list for this week. When data delivery is targetted to a thread pool other than the one that owns the StreamFilter worker, the data chunks sometimes wind up being processed out of order. I need to figure out a safe locking arrangement that avoids that problem.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Hmm, looking at a debug build, though, shows that StreamFilterParent hit an assertion:

https://treeherder.mozilla.org/logviewer.html#?job_id=133326989&repo=mozilla-inbound&lineNumber=39676

Kris, sorry to bother you again, but does that make any sense to you?  Shouldn't fetch() be in the client side here?

If this is a known issue in StreamFilter, should we perhaps mark it not safe for ThreadRetargeting?

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Or is there another solution so I can land this fetch() change if this is a known issue in webrequest?

Comment 25

[Kris Maglione [:kmag]]

(In reply to Ben Kelly [:bkelly] from comment #23)
> Shouldn't fetch() be in the client side here?

No, that arrangement here is kind of unusual. The parent side is always in the
process that the request originated in (i.e., the process that owns the
HttpChannelChild), and the child side is always in the extension process.

The parent side used to always be in the parent process, but for complicated
reasons related to content decoders, it's really difficult to correctly filter
response data from the parent process.

> If this is a known issue in StreamFilter, should we perhaps mark it not safe
> for ThreadRetargeting?

We could, but I'd rather just fix it correctly. Like I said, it's on my todo
list for this week.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #25)
> We could, but I'd rather just fix it correctly. Like I said, it's on my todo
> list for this week.

Ok, can you make that bug block this one?

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Don't fire FetchDriverObserver::OnDataAvailable() for every FetchDriver ODA callback. r=baku

Andrea, this patch makes us only trigger the FetchDriverObserver::OnDataAvailable() callback if its actually needed to update a FetchObserver.  In addition, we only call it on the first ODA.

The goal is to remove as many main thread runnables as possible while receiving data.  Unfortunately the OnDataAvailable() code was sending an additional MT runnable for every ODA which kind of defeated the point of retargeting OMT.

Comment 28

[Andrea Marchesini [:baku]]

Comment on attachment 8917842 [details] [diff] [review]
P2 Don't fire FetchDriverObserver::OnDataAvailable() for every FetchDriver ODA callback. r=baku

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

::: dom/fetch/Fetch.cpp
@@ +508,5 @@
> +bool
> +MainThreadFetchResolver::NeedOnDataAvailable()
> +{
> +  NS_ASSERT_OWNINGTHREAD(MainThreadFetchResolver);
> +  return mFetchObserver;

return !!mFetchObserver;
just to make it clear that we are returning a boolean.

@@ +715,5 @@
> +WorkerFetchResolver::NeedOnDataAvailable()
> +{
> +  AssertIsOnMainThread();
> +  MutexAutoLock lock(mPromiseProxy->Lock());
> +  return mFetchObserver;

here as well.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Don't fire FetchDriverObserver::OnDataAvailable() for every FetchDriver ODA callback. r=baku

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Hmm, the try run is suggesting that maybe the webrequest test is not failing any more with fetch() OMT retargetting.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Oh, re-triggers show its clear still a problem.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Disable OMT retargeting of fetch() requests while being traced until webrequest can be fixed. r=kmag

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8917940 [details] [diff] [review]
P3 Disable OMT retargeting of fetch() requests while being traced until webrequest can be fixed. r=kmag

Kris, I understand you have this problem on your todo list, but I'd like to avoid letting this bug sit for any longer.  I'd like to land OMT delivery now while you continue to work on the webrequest issue.

To that end, what do you think about this?  It adds a way to detect if channel tracing is being used and avoids thread retargeting in that case.

This of course disables retargeting when devtools network monitor is active as well, but that should be ok for the near term.  In the long run we will remove this restriction.

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

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Try is green with P3.  I did 10 retriggers and could not reproduce the previous failure.

Comment 36

[Kris Maglione [:kmag]]

Comment on attachment 8917940 [details] [diff] [review]
P3 Disable OMT retargeting of fetch() requests while being traced until webrequest can be fixed. r=kmag

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

If we're going to do this, let's just return failure in StreamFilterParent::CheckListenerChain()

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Disable channel thread retargeting when webrequest is tracing the channel. r=kmag

Ok.  That does seem best given other consumers try to retarget channels as well.  Sorry to push for this right now, but it should be easily reversed once the problem is solved.  Thanks.

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

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0db1445c1db9
FetchDriver should have threadsafe refcounting. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/839e4b1245ca
P2 Don't fire FetchDriverObserver::OnDataAvailable() for every FetchDriver ODA callback. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/213cba8335fd
P3 Disable channel thread retargeting when webrequest is tracing the channel. r=kmag

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0db1445c1db9
https://hg.mozilla.org/mozilla-central/rev/839e4b1245ca
https://hg.mozilla.org/mozilla-central/rev/213cba8335fd