Closed
Bug 1405506
Opened 7 years ago
Closed 7 years ago
StreamFilter.disconnect does not flush data that was buffered in the child
Categories
(WebExtensions :: Request Handling, enhancement, P3)
Tracking
(firefox57 wontfix, firefox58 verified)
VERIFIED
FIXED
mozilla58
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
Reporter | ||
Comment 1•7 years ago
|
||
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.
Reporter | ||
Comment 2•7 years ago
|
||
(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).
Updated•7 years ago
|
status-firefox57:
--- → wontfix
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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 | ||
Updated•7 years ago
|
Assignee: nobody → kmaglione+bmo
Assignee | ||
Comment 5•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/95cfe19746a3b03a5ea6ed946a0e3e38d99264e0 Bug 1405506: Flush buffered data when disconnecting suspended channel. r=mixedpuppy
Assignee | ||
Updated•7 years ago
|
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/95cfe19746a3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 7•6 years ago
|
||
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.
Updated•6 years ago
|
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Comment 8•6 years ago
|
||
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•