The default bug view has changed. See this FAQ.

Support a webRequest.onResponseData event to filter HTTP response bytes as they come

NEW
Assigned to

Status

()

Toolkit
WebExtensions: Request Handling
P2
normal
a year ago
an hour ago

People

(Reporter: mao, Assigned: kmag, NeedInfo)

Tracking

(Blocks: 2 bugs)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webRequest] triaged, [we-enterprise])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments, 2 obsolete attachments)

59 bytes, text/x-review-board-request
kmag
: review?
dragana
Details | Review
59 bytes, text/x-review-board-request
kmag
: review?
mixedpuppy
Details | Review
59 bytes, text/x-review-board-request
kmag
: review?
billm
Details | Review
59 bytes, text/x-review-board-request
kmag
: review?
mixedpuppy
kmag
: review?
billm
Details | Review
59 bytes, text/x-review-board-request
kmag
: review?
dragana
kmag
: review?
mixedpuppy
Details | Review
59 bytes, text/x-review-board-request
kmag
: review?
mixedpuppy
kmag
: review?
billm
Details | Review
59 bytes, text/x-review-board-request
kmag
: review?
mixedpuppy
Details | Review
59 bytes, text/x-review-board-request
kmag
: review?
mixedpuppy
Details | Review
(Reporter)

Description

a year ago
Extensions like NoScript or  EPUBReader ( https://addons.mozilla.org/en-US/firefox/addon/epubreader ), whose developer has recently contacted me to investigate the feasibility of a WebExtensions migration, need to manipulate HTTP responses on the fly and possibly divert them from their original consumer.

The Chrome extensions API does support a requestBody property in the webRequest.onBeforeRequest event (which, strangely enough, represents raw data a single array of bytes, with no way to consume it in chunks when it's a very large upload), allows for request and response headers manipulation, but has no way to analyze, let alone modify, the response payload.

Like requestBody, responseChunk be potentially very large and its consumers may require asynchronous processing not to block the parent process, therefore this too depends on bug 1254204.



Therefore I propose to extend the webRequest API with an additional event, tentatively called onResponseData, which is called one or more times as the response data is streamed down. The details object would contain the usual properties, and especially requestId, allowing clients to keep per-request state as needed, plus a new property called responseChunk, i.e. an array filled with the response bytes in chunks, as they're made available to the onDataAvailable() method of nsIStreamListener attached to the underlying HTTP channel.

Since the webRequest API guarantees that either the onCompleted or the onErrorOccurred listener is called when a request is done, we don't need any special EOF signaling: the client would handle its cleanup operations in those events' callbacks.
(Reporter)

Updated

a year ago
No longer blocks: 1201979

Updated

11 months ago
Whiteboard: [webRequest] → [webRequest] triaged

Updated

11 months ago
Flags: blocking-webextensions+
Priority: -- → P2

Updated

11 months ago
Flags: blocking-webextensions+

Comment 1

8 months ago
Please update us on the progress of onResponseData implementation

Our add-on needs to read incoming data on the fly

Updated

5 months ago
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Blocks: 1310316
We need to make sure this work doesn't cause performance problems.  We've done a lot of work in necko to allow off-main thread delivery (see bug 497003)--currently the HTML parser and the image parser both use that to improve performance.  Currently that work gets disabled if we detect that the nsITraceableChannel (the standard--and IIUC the only--way to intercept and modify data before OnDataAvailable is called on the original listener). See bug 908398 and bug 908405)

So we want to 1) discourage use of this API unless it's really needed (and I get that it may be for some use cases), and 2) make sure that we don't bake in any performance hit when these addons aren't being used.  I'm guessing we'll get #2 for free--that's how things already work (necko does a search through the listener chain to make sure all listeners are off-main-ready before switching to off-main delivery: the presence of any JS-implemented listeners (added by nsITraceableChannel), for instance, causes code to stay main-thread only).

So maybe there's no issue here--but I want to make sure. SC, can you keep an eye on the architecture used here and make sure we don't break off-main delivery any more than necessary?  If you need help figuring that out ask Honza.
Flags: needinfo?(schien)
(Assignee)

Comment 3

4 months ago
The extension code that's consuming this will be running in its own process, so we can probably implement it in a way that's compatible with off-main-thread delivery. Either way, I definitely agree that we want to discourage this whenever it's not absolutely necessary.

Comment 4

4 months ago
OOP addons are still single-threaded, maybe it should be handled by workers?
(Assignee)

Comment 5

4 months ago
(In reply to The 8472 from comment #4)
> OOP addons are still single-threaded, maybe it should be handled by workers?

It's not so much an issue of them being single-threaded as it is of not blocking the main UI thread while the data is being processed. Granted, they'd still block any UI that's running in the extension process, but that's a much smaller issue.

It definitely would be nice to be able to push these kinds of APIs into something like a service worker in the future, but that's a ways down the road.

Comment 6

4 months ago
Not quite as simple. If an extension takes time to mangle each request this can also slow down page loads. Parallelizing it could improve user experience.
(Assignee)

Comment 7

4 months ago
If the data processing takes longer than the data transfer from the network, that's probably true. But workers don't buy us anything unless we somehow spawned multiple workers to handle parallel requests (which presents its own whole set of problems), and we'd like to discourage extensions from doing things like that in any case.

Comment 8

4 months ago
Yes, the idea was to have multiple workers. But even with a single worker it would still improve throughput if multiple addons use this feature.
(Assignee)

Comment 9

4 months ago
Only if they happen to be running in the same process. But, again, if we have more consumers of this API than we have extension processes, or those extensions process data significantly slower than we can read it from the network, we're doing something wrong. That's not how this API is meant to be used.
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1325278

Comment 11

3 months ago
This bug is blocking me from porting my extension to WebExtensions. I have an extension that scans JPEGs that are coming over the wire to alert on them containing EXIF GPS information (which is potentially privacy-compromising). My extension currently uses an nsIObserver and snoops incoming responses with JPEG MIME types. As far as I can tell, there isn't a way to do this using WebExtensions.

The source is here: https://github.com/allanlw/gps-detect/ and the listing is: https://addons.mozilla.org/en-US/firefox/addon/gpsdetect/

Updated

2 months ago
webextensions: --- → ?

Updated

2 months ago
Assignee: g.maone → nobody

Updated

a month ago
Whiteboard: [webRequest] triaged → [webRequest] triaged, [we-enterprise]
(Assignee)

Updated

a month ago
Duplicate of this bug: 1340802

Comment 13

a month ago
Kris - Thanks for merging the bugs. My addon Integrated Inbox will cease to work, as it sounds like is the case for many others, unless this is addressed. 

Have a great weekend,
Michael
(Assignee)

Updated

17 days ago
Duplicate of this bug: 1344561

Comment 15

14 days ago
Created attachment 8845654 [details] [diff] [review]
webrequest_oda_uint8.diff

Would something like this be what we're looking for, once bug 1261585 is fixed?

This patch just adds an onResponseData listener to WebRequest, which can optionally be "blocking" if response-rewriting is desired rather than simple inspection. It passes the data into the listener as a Uint8Array named responseBytes.

It just builds on the existing StartStopListener::OnDataAvailable, only doing extra work if an onResponseData listener was detected (hopefully I did that correctly).
Attachment #8845654 - Flags: review?(kmaglione+bmo)

Comment 16

12 days ago
Created attachment 8846323 [details] [diff] [review]
webrequest_oda_uint8.diff

Here's a fresh version of the last patch, which changes the onDataAvailable handler to be a bit more solid, now that I've had a chance to test it a bit more thoroughly.
Attachment #8845654 - Attachment is obsolete: true
Attachment #8845654 - Flags: review?(kmaglione+bmo)
Attachment #8846323 - Flags: feedback?(kmaglione+bmo)
I started to look at bug 1326298 and was going to put some focus on that.  I'd rather not see this land simply to have to convert it to native code right away.
(Assignee)

Comment 18

an hour ago
Comment on attachment 8846323 [details] [diff] [review]
webrequest_oda_uint8.diff

Review of attachment 8846323 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I didn't see this sooner. As commented elsewhere in the bug, this needs to be implemented in C++, with careful attention to thread safety and performance. I thought the bug was already assigned to me.
Attachment #8846323 - Flags: feedback?(kmaglione+bmo) → feedback-
(Assignee)

Updated

an hour ago
Assignee: nobody → kmaglione+bmo
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

an hour ago
Comment on attachment 8846323 [details] [diff] [review]
webrequest_oda_uint8.diff

Oh, no problem! One way or the other, I figured it would be worth providing a patch for others to experiment with if they need such an API for their extensions.
Attachment #8846323 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.