1645941 - data race StreamFilterParent::mState in OnStartRequest and ODA

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P2)

Description

[Junior [inactive]]

https://searchfox.org/mozilla-central/rev/027893497316897b8f292bde48dbb6da2391a331/toolkit/components/extensions/webrequest/StreamFilterParent.cpp#537

https://searchfox.org/mozilla-central/rev/027893497316897b8f292bde48dbb6da2391a331/toolkit/components/extensions/webrequest/StreamFilterParent.cpp#633

These two are obviously racing, r/w mState should be in actor thread.

Comment 1

[Junior [inactive]]

Attachment: Bug 1645941 - Use Release-Acquire ordering to ensure the OMT ODA is handled correctly, r=mixedpuppy

To resolve the race of r/w mState, move checking state to actor thread.
It sacrifices the performance when the filter is closed during ODA, which might be better than adding mutex.

Depends on D79771

Comment 2

[Junior [inactive]]

needinfo? for moving this forward given it's hanging one week

Comment 3

[Junior [inactive]]

To be more detailed,
StreamFilterParent::OnStartRequest dispatches mState setting to actor thread.
https://searchfox.org/mozilla-central/rev/3d39d3b7dd1b2be30692d4541ea681614e34c786/toolkit/components/extensions/webrequest/StreamFilterParent.cpp#537

which is racy with the following OnDataAvailabe which checks mState in IO thread.
https://searchfox.org/mozilla-central/rev/3d39d3b7dd1b2be30692d4541ea681614e34c786/toolkit/components/extensions/webrequest/StreamFilterParent.cpp#633

mutex is possibly more preferred around the gecko, but I'd like to have more information to move it forward.

Comment 4

[Junior [inactive]]

This is another approach to use Atomic instead of volatile
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b727ec7c0168f08671b3caf759a4d4fe2b29661

Comment 5

[Junior [inactive]]

(In reply to Junior [:junior] from comment #4)

This is another approach to use Atomic instead of volatile
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b727ec7c0168f08671b3caf759a4d4fe2b29661

This works well with better semantic. Switch to this approach.

Comment 6

[Denis Lisov]

Do I understand correctly that you leave the actual logical race for someone else to handle? As you could read in bug 1644621, switching the state to Disconnected can and does happen when a chunk of data is in flight between ODA and DoSendData (i.e. when the DoSendData callback has been enqueued but not executed yet) and the if (mState == State::TransferringData) check in DoSendData drops that chunk of response without delivering it anywhere.

Comment 7

[Junior [inactive]]

(In reply to Denis Lisov from comment #6)

Do I understand correctly that you leave the actual logical race for someone else to handle? As you could read in bug 1644621, switching the state to Disconnected can and does happen when a chunk of data is in flight between ODA and DoSendData (i.e. when the DoSendData callback has been enqueued but not executed yet) and the if (mState == State::TransferringData) check in DoSendData drops that chunk of response without delivering it anywhere.

What I'd like to handle is to avoid race between two call points in comment 0: OnStartRequest and ODA.
We don't race mState between OnStartRequest and DoSendData given the order is guaranteed and they are on the same thread.

FinishDisconnect setting mDisconnected between ODA and DoSendData might be another problem.
We probably want OnStopRequest to ensure if it's good to disconnect. I'll leave the detail to extension team.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b345ef2710e
Use Release-Acquire ordering to ensure the OMT ODA is handled correctly, r=mixedpuppy

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/0b345ef2710e