Closed Bug 1405506 Opened 2 years ago Closed 2 years ago

StreamFilter.disconnect does not flush data that was buffered in the child

Categories

(WebExtensions :: Request Handling, enhancement, P3)

57 Branch
enhancement

Tracking

(firefox57 wontfix, firefox58 verified)

VERIFIED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: robwu, Assigned: kmag)

References

Details

Attachments

(3 files)

From the extension's perspective, when filter.suspend() is called, the extension is not expected to handle any data.
So if an extension writes previously received data and calls filter.disconnect(), then I would expect all data to be sent normally.

filter = browser.webRequest.filterResponseData(requestId);
let chunks = [];
filter.ondata = ({data}) => {
    filter.suspend();
    chunks.push(data);
};

// Later:
for (let chunk of chunks) {
  filter.write(buf);
}
filter.disconnect();
// Expected: All data is sent to the page.
// Actual: It is not.



When StreamFilterChild::mState == State::Suspended, mBufferedData can be non-empty (e.g. due to StreamFilterChild::RecvData [1]).

When StreamFilterChild::Disconnect is invoked, I expect any buffered data to be flushed before the channel is resumed.

However, when I follow what happens upon calling filter.disconnect(), I see:

1. StreamFilterChild::Disconnect
  -> mState = State::Disconnecting (was: Suspended)
  -> mNextState = State::Disconnected
  -> SendDisconnect()

2. StreamFilterParent::RecvDisconnect
  -> mChannel->Resume()
  -> mState = State::Disconnecting (was: Suspended)
  -> SendFlushData()

3. StreamFilterChild::RecvFlushData
  -> SendFlushedData()
  -> mState = State::Disconnected (was: Disconnecting)

4. StreamFilterParent::RecvFlushedData
  -> Destroy()
  -> StreamFilterParent::FlushBufferedData()
  -> mState = State::Disconnected
5. StreamFilterParent::FlushBufferedData
  -> Flushes mBufferedData from StreamFilterParent.


During this ping-ponging, mBufferedData in StreamFilterParent is not touched at all. Consequently, when a filter is suspended and disconnected, the first chunks of a response body may be lost.

StreamFilterChild::FlushBufferedData is the only method that removes from mBufferedData in the child (and dispatches the chunks to the extension via the filter.ondata event). This is called from:
- StreamFilterChild::Resume
  -> if mState = State::Suspended , Suspending , Resuming or TransferringData
- StreamFilterChild::SetNextState
  -> if mState = State::TransferringData (via RecvResumed)

However, I cannot do this either:
filter.resume(); // would synchronously call filter.ondata so we could call filter.write
filter.disconnect();
...because StreamFilterChild::Resume puts mNextState in State::TransferringData, while StreamFilterChild::Disconnect throws when in this state.


[1] https://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/toolkit/components/extensions/webrequest/StreamFilterChild.cpp#498-501
STR:

1. Load extension at about:debugging.
2. Click on the extension button to open
https://upload.wikimedia.org/wikipedia/commons/f/f4/Firefox_on_Windows_10.png?disco...
3. The extension will suspend the filter on the first data event, and disconnect() after two seconds.

Expected:
- The image should be displayed (remove ?disco... to see the expected image).

Actual:
- Image is corrupted because parts of the response body were lost.


Note: If you get the expected result at the first try, then you have been lucky. Click the extension button again, or try to load a different URL containing "disco" that would respond with a large response body.
(In reply to Rob Wu [:robwu] from comment #0)
> During this ping-ponging, mBufferedData in StreamFilterParent is not touched

Of course I meant "mBufferedData in StreamFilterChild".

This bug also applies to the child in State::Suspending (RecvData will also buffer messages when in this state).

It seems that this bug can be solved by including the content of mBufferedData in the SendDisconnect IPC call (or just calling SendWrite for every element in mBufferedData before calling SendDisconnect in Disconnect/SetNextState).
Priority: -- → P3
Depends on: 1398120
Comment on attachment 8924034 [details]
Bug 1405506: Flush buffered data when disconnecting suspended channel.

https://reviewboard.mozilla.org/r/195248/#review200698
Attachment #8924034 - Flags: review?(mixedpuppy) → review+
Assignee: nobody → kmaglione+bmo
https://hg.mozilla.org/integration/mozilla-inbound/rev/95cfe19746a3b03a5ea6ed946a0e3e38d99264e0
Bug 1405506: Flush buffered data when disconnecting suspended channel. r=mixedpuppy
Blocks: 1398120
No longer depends on: 1398120
https://hg.mozilla.org/mozilla-central/rev/95cfe19746a3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Verified as fixed using Build identifier: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0. Please see postfix screenshot.
Status: RESOLVED → VERIFIED
Attached image Postfix screenshot
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.