Closed Bug 1638422 Opened 4 years ago Closed 4 years ago

Page with multipart/x-mixed-replace content doesn't finish loading when certain extensions are loaded

Categories

(WebExtensions :: General, defect, P2)

76 Branch
defect

Tracking

(firefox-esr68 unaffected, firefox-esr78 wontfix, firefox76 wontfix, firefox77 wontfix, firefox78 wontfix, firefox79 wontfix, firefox80 wontfix, firefox81 wontfix, firefox82 fixed)

RESOLVED FIXED
82 Branch
Tracking Status
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.

  1. Open firefox (with a clean profile)
  2. Install certain extensions (see below for a list of the ones I tested)
  3. navigate to https://bugzilla.kernel.org/ (the version of bugzilla here on mozilla.org doesn't seem to trigger the bug for me)
  4. 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.

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Product: Firefox → WebExtensions

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

Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → General
Ever confirmed: true
Keywords: regression
Regressed by: 1600211
Summary: page doesn't finish loading when certain extensions are loaded → Page with multipart/x-mixed-replace content doesn't finish loading when certain extensions are loaded

Hey Matt, can you please take a look.

Flags: needinfo?(matt.woodrow)
Severity: -- → S2
Priority: -- → P2
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)

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

Flags: needinfo?(tomica)

I'm not sure multiple onStart/onStop events per StreamFilter would work for compatibility.

Shane is much more knowledged with details here.

Flags: needinfo?(tomica) → needinfo?(mixedpuppy)

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?

Flags: needinfo?(mixedpuppy) → needinfo?(matt.woodrow)

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

Is 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?

Flags: needinfo?(matt.woodrow)

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

(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

Flags: needinfo?(kmaglione+bmo)

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.

Flags: needinfo?(mixedpuppy)

This isn't necessary any more, since we now attach StreamFilters directly to the 'real' channel when we replace DocumentChannel.

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.

Flags: needinfo?(mixedpuppy) → needinfo?(matt.woodrow)

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.

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

Flags: needinfo?(matt.woodrow)
Flags: needinfo?(kmaglione+bmo)
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

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Are we going to land the test still?

Flags: needinfo?(matt.woodrow) → needinfo?(mixedpuppy)
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/84e7233d29a6
test multipart responses with filterResposeData r=mattwoodrow
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a5d99bad98c
test multipart responses with filterResposeData r=mattwoodrow
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(mixedpuppy)

Bug 1640573 was marked as duplicated of this bug but this bug is still present in firefox-82 so this is not fixed.

Wait, used wrong version to verify, will shortly re-test with real 82.x...

It's fixed in firefox-82 RC2, sorry for the noise!

Regressions: 1683189
You need to log in before you can comment on or make changes to this bug.