Page with multipart/x-mixed-replace content doesn't finish loading when certain extensions are loaded
Categories
(WebExtensions :: General, defect, P2)
Tracking
(firefox-esr68 unaffected, firefox-esr78 wontfix, firefox76 wontfix, firefox77 wontfix, firefox78 wontfix, firefox79 wontfix, firefox80 wontfix, firefox81 wontfix, firefox82 fixed)
People
(Reporter: jadevree, Assigned: mattwoodrow)
References
(Regression)
Details
(Keywords: regression)
Attachments
(5 files)
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:76.0) Gecko/20100101 Firefox/76.0
Steps to reproduce:
This is like https://bugzilla.mozilla.org/show_bug.cgi?id=1622490 but instead of happening when devtools is open, it happens if certain extensions are loaded.
- Open firefox (with a clean profile)
- Install certain extensions (see below for a list of the ones I tested)
- navigate to https://bugzilla.kernel.org/ (the version of bugzilla here on mozilla.org doesn't seem to trigger the bug for me)
- Search for anything, I used "megaraid"
Extensions which can trigger the issue:
- uBlock Origin
- Firefox Multi-Account Containers
- CanvasBlocker
Extensions which do not trigger the issue:
- Proxy Switcher and Manager
- Reddit Enhancement Suite
The common element in the permissions is "Access your data for all websites".
Actual results:
page renders, but the loading icon on the tab continues forever.
Expected results:
Page should have finished loading.
Comment 1•4 years ago
|
||
Bugbug thinks this bug should belong to this component, but please revert this change in case of error.
Comment 2•4 years ago
|
||
I can reproduce the issue on Nightly78.0a1 Windows10 with 'Firefox Multi-Account Containers'.
Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fe94358096926edc1404cdd2e864ce66a38fc91f&tochange=1e479194244ab14e9ebad664865c8e6bc2cdf1c7
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 5•4 years ago
|
||
This fails because ChannelWrapper::RequestListener doesn't implement and forward nsIMultiPartChannelListener, so we lose our notifications that all parts of a multi-part channel are complete.
There's a bigger issue here though, of how we want to handle multi-part responses in the presence of StreamFilter.
Bug 1600211 made us handle multipart decoding in the parent process, so we'll be calling onResponseStarted/onCompleted for each part, not just once for the whole response.
I believe StreamFilter onstart/onstop will also be called for each part (as long as the extension doesn't disconnect/close at the end of the first part).
Is this the behaviour we want, and is it acceptable for backward compatibility?
It's possible that we instead need to listen to the http channel before the multipart processing happens, so that we continue to see only a single response part (with all the part delimiters etc contained in the response).
Comment 6•4 years ago
|
||
I'm not sure multiple onStart/onStop events per StreamFilter would work for compatibility.
Shane is much more knowledged with details here.
Comment 7•4 years ago
•
|
||
Well it looks like we need to implement nsIMultiPartChannelListener, but need to decide on if/how that is exposed to extensions.
Right now all the event handlers get an event object:
https://searchfox.org/mozilla-central/source/dom/webidl/StreamFilterDataEvent.webidl
Is there any data that could be added for these to indicate anything about thee part that is being processed?
Is it possible that different parts may be processed at the same time? e.g.
onstart (part 1)
onstart (part 2)
ondata (part 1)
ondata (part 2)
onstop (part 1)
ondata (part 2)
onstop (part 2)
If so, I think it's important to have some extra data on the event object. If it all happens synchronously, then we could just document that.
During onAfterLastPart we could issue an additional onstop with some flag that the request is complete, or we could add something like a StreamFilter.onMultiPartComplete event.
Matt, do you have any thoughts or input on the above?
Assignee | ||
Comment 8•4 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
Well it looks like we need to implement nsIMultiPartChannelListener, but need to decide on if/how that is exposed to extensions.
Right now all the event handlers get an event object:
https://searchfox.org/mozilla-central/source/dom/webidl/StreamFilterDataEvent.webidlIs there any data that could be added for these to indicate anything about thee part that is being processed?
Yeah, nsIMultiPartChannel has the current part number that's being processed. It also has 'isLastPart', but this isn't always accurate, and so I think we should avoid exposing it further.
Is it possible that different parts may be processed at the same time? e.g.
onstart (part 1)
onstart (part 2)
ondata (part 1)
ondata (part 2)
onstop (part 1)
ondata (part 2)
onstop (part 2)If so, I think it's important to have some extra data on the event object. If it all happens synchronously, then we could just document that.
No, we always process parts serially, and won't get onstart for a latter part until the previous part has had onstop.
During onAfterLastPart we could issue an additional onstop with some flag that the request is complete, or we could add something like a StreamFilter.onMultiPartComplete event.
Matt, do you have any thoughts or input on the above?
Either of those would be fine!
My main concern is that there's possibility of breakage with existing addons. If there are multiple parts, and a stream filter closes or disconnects from the onstop of the first part, what should we do with the second part?
Updated•4 years ago
|
Comment 9•4 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
My main concern is that there's possibility of breakage with existing addons. If there are multiple parts, and a stream filter closes or disconnects from the onstop of the first part, what should we do with the second part?
A disconnect shouldn't matter, the stream should just continue without calling into the extension. The close should end the stream which is probably problematic. We could add an option when creating the streamfilter that needs to be used in order to get multiple parts.
It's possible that we instead need to listen to the http channel before the multipart processing happens, so that we continue to see only a single response part (with all the part delimiters etc contained in the response).
How difficult is that, and do we loose anything by doing so? This would probably be the most compatible, but maybe slightly less flexible for extensions than having start/stop per part.
Could we do both (and is there value in it)? Default to send the full stream, if the extension specifies support for multipart, start/stop for each as is done now.
var filter = browser.webRequest.filterResponseData(
requestId // string
acceptMultipart // boolean default false
)
Assignee | ||
Comment 10•4 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
It's possible that we instead need to listen to the http channel before the multipart processing happens, so that we continue to see only a single response part (with all the part delimiters etc contained in the response).
How difficult is that, and do we loose anything by doing so? This would probably be the most compatible, but maybe slightly less flexible for extensions than having start/stop per part.
Could we do both (and is there value in it)? Default to send the full stream, if the extension specifies support for multipart, start/stop for each as is done now.
var filter = browser.webRequest.filterResponseData( requestId // string acceptMultipart // boolean default false )
Listening in the parent process, before multi-part handling would solve this, and also mean that file downloads are intercepted. Unfortunately it'd also happen before content conversion (gzip etc). I'm not sure we want to do that in the parent process for all compressed pages when adblock is installed.
I guess we could attempt to intercept in the parent process, and then defer to the child once we detect that the content type isn't multipart. We're already doing decompression in the parent for compressed multi-part, so that's not a regression.
It's also possible that we could run the decompression in the extension process, but I think we'd have to route all bytes through the ext process (even after a disconnect) for things to be sane.
Adding ni?kmag in case he has any other ideas
Updated•4 years ago
|
Assignee | ||
Comment 11•4 years ago
|
||
Shane, do you think you could write tests for this?
I'm fixing the code, and can confirm that it fixes the reported issue, but I haven't yet managed to write a test that reproduces it.
It looks like uBlock Origin/CanvasBlocker aren't actually using filterResponseData, but are still broken because ChannelWrapper::RequestListener doesn't support multipart.
The above discussion about how to properly support multipart in filterResponseData is still relevant though, just less common.
Assignee | ||
Comment 12•4 years ago
|
||
This isn't necessary any more, since we now attach StreamFilters directly to the 'real' channel when we replace DocumentChannel.
Assignee | ||
Comment 13•4 years ago
|
||
Depends on D83594
Assignee | ||
Comment 14•4 years ago
|
||
Depends on D83595
Assignee | ||
Comment 15•4 years ago
|
||
Depends on D83596
Comment 17•4 years ago
|
||
Comment 18•4 years ago
|
||
I'm not entirely sure what I should be expecting in the api but I've attached a test that gets us most of the way to having one.
Comment 19•4 years ago
|
||
Matt, lets discuss what should be in the multipart tests, and what I should expect to happen in the api, then we can finish up the test.
Assignee | ||
Comment 20•4 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
I'm not entirely sure what I should be expecting in the api but I've attached a test that gets us most of the way to having one.
The test looks good to me!
The current patches should be reverting to the previous behaviour where we single a single onstart/onstop pair for multipart responses, and extensions will be able to see the boundaries.
We still could add an option for extensions to opt into getting the processed parts separately, but we should handle that in a new bug.
Updated•4 years ago
|
Comment 22•4 years ago
|
||
Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/89a7f6237dd6 Remove nsITraceableChannel from DocumentChannel. r=kmag,necko-reviewers https://hg.mozilla.org/integration/autoland/rev/25dcbf81738a Add an option for nsITraceableChannel new listeners to request that content decoding be applied before they are called. r=kmag,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/ffbc455f1a9f Handle attaching StreamFilters to multipart responses from the parent process, so that they see the unparsed single stream. r=kmag,necko-reviewers,dragana https://hg.mozilla.org/integration/autoland/rev/8a2f6318ffa3 Implement nsIMultiPartChannelListener for RequestListener. r=kmag
Comment 23•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/89a7f6237dd6
https://hg.mozilla.org/mozilla-central/rev/25dcbf81738a
https://hg.mozilla.org/mozilla-central/rev/ffbc455f1a9f
https://hg.mozilla.org/mozilla-central/rev/8a2f6318ffa3
Comment 24•4 years ago
|
||
Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Comment 26•4 years ago
|
||
Backed out for xpcshell failures on test_ext_webRequest_responseBody.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/d23c723df4831ab536b1563e8383709351a1ce81
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=314520047&repo=autoland&lineNumber=6961
Comment 27•4 years ago
|
||
Pushed by cbrindusan@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/84e7233d29a6 test multipart responses with filterResposeData r=mattwoodrow
Comment 28•4 years ago
|
||
Pushed by scaraveo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9a5d99bad98c test multipart responses with filterResposeData r=mattwoodrow
Comment 29•4 years ago
|
||
bugherder |
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Comment 30•4 years ago
|
||
Bug 1640573 was marked as duplicated of this bug but this bug is still present in firefox-82 so this is not fixed.
Comment 31•4 years ago
|
||
Wait, used wrong version to verify, will shortly re-test with real 82.x...
Comment 32•4 years ago
|
||
It's fixed in firefox-82 RC2, sorry for the noise!
Description
•