Closed Bug 1645941 Opened 4 years ago Closed 4 years ago

data race StreamFilterParent::mState in OnStartRequest and ODA

Categories

(WebExtensions :: Request Handling, defect, P2)

defect

Tracking

(firefox79 fixed)

RESOLVED FIXED
mozilla79
Tracking Status
firefox79 --- fixed

People

(Reporter: CuveeHsu, Assigned: CuveeHsu)

References

Details

Attachments

(1 file)

Blocks: 1633935

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

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

Flags: needinfo?(kmaglione+bmo)

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.

Attachment #9156873 - Attachment description: Bug 1645941 - defer state check to actor thread in StreamFilterParent::ODA, r=kmag → Bug 1645941 - Use Release-Acquire ordering to ensure the OMT ODA is handled correctly, r=kmag

(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.

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.

See Also: → 1629299
Severity: -- → S2
Priority: -- → P2

(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.

Attachment #9156873 - Attachment description: Bug 1645941 - Use Release-Acquire ordering to ensure the OMT ODA is handled correctly, r=kmag → Bug 1645941 - Use Release-Acquire ordering to ensure the OMT ODA is handled correctly, r=mixedpuppy
Pushed by juhsu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0b345ef2710e
Use Release-Acquire ordering to ensure the OMT ODA is handled correctly, r=mixedpuppy
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.