Closed Bug 1631933 Opened 2 months ago Closed 2 months ago

Remove or revisit support for data:-URLs in the webRequest API (remove/replace WebRequestContent.js)

Categories

(WebExtensions :: Request Handling, task)

task

Tracking

(firefox77 fixed)

RESOLVED FIXED
mozilla77
Tracking Status
firefox77 --- fixed

People

(Reporter: robwu, Assigned: robwu, NeedInfo)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, devrel-needed)

Attachments

(2 files)

The support for data:-URIs in the webRequest is very limited: webRequest.onBeforeRequest (and webRequest.onCompleted) are the only supported events, and only in an observational capacity (i.e. it is not possible to block data:-URL requests, which means that the API has little value for content blockers, such as NoScript and uBlock; I checked their sources and didn't see any special handling for it).

Despite that, there is a significant amount of code to support this, in WebRequestContent.js and elsewhere. The code has bugs, e.g. with Fission enabled data:-requests are not propagated from cross-origin frames.

Originally WebRequestContent.js was used as the backend for webRequest events. These days, it is not relevant any more:

  • http(s) and ws(s) are handled in the parent (as of bug 1163862)
  • file:-URLs are not supported (bug 1341341). Even if it were, it would better be handled in the parent process.
  • data:-URL support is very limited (this bug, and note that Chromium doesn't support it).
  • ftp: is not supported and is not going to be (bug 1312460).

Given the very limited use of data:-URLs, I propose to drop support for data:-URLs in the webRequest API, to enable us to clean up some code and fix (perf) bugs.

Blocks: 1630999
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/3916697cfe0b
Drop support for data:-URLs in webRequest r=mixedpuppy
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

Hey Philipp, we should probably announce this in the next blog post.

Flags: needinfo?(philipp)
Keywords: devrel-needed

I can understand why the code needs cleanup, thanks to the patient response of Shane over on #1475832. However, I'd like to put in a plea for eventually having data: URL's fully supported in webRequest. Unlike ftp: or file:, data: is regularly encountered during normal browsing sessions.

As a real use case: I develop a plugin that blocks NSFW images by scanning them and my only recourse for scanning/blocking data: URI's is to 1) scan all HTML documents on the way in and then 2) extract and if objectionable, replace the data URI by regex. (You can see the abominable code in action here ) Unfortunately things like the "above the fold" images from the first page of a Google Images search are all data: URI's so its not exactly a great option for me to turn it off. As you can imagine, finding that data: URI interceptions were limited to read-only as noted above was a big disappointment.

At any rate, thank you for the work that you do! I have not lost anything by the removal of the current functionality, and am simply looking forward with great interest to further developments around webRequest.

We should probably verify that any mention of data: in webrequest docs is updated to reflect that data: is not supported.

Keywords: dev-doc-needed

Listening for data:-URL requests was never documented. Although data: is listed in match patterns, the webRequest documentation explicitly excluded data:-URLs with the following statement in webRequest.requestFilter:

An array of match patterns. The listener will only be called for requests whose targets match any of the given patterns. Only requests made using HTTP or HTTPS will trigger events, even though match patterns can match some other protocols.

I don't expect significant functional regressions as a result of the removal, because it's not documented and because of its history (and the fact that Chromium doesn't support it):

  • In bug 1163862 (Firefox 47), the feature was introduced for a legacy version of NoScript (with ability to block data:-URLs, but only if onBeforeRequest was handled synchronously).
  • In bug 1305217 (Firefox 52), the synchronous blocking part stopped working for WebExtensions because webRequest events were now handled asynchronously.
  • In bug 1393909 (Firefox 57), the synchronous blocking part was removed because there were no consumers left.

If anyone cared about blocking/modifying data:-URLs, then they would have complained after the release of Firefox 52 or 57, but that didn't happen until one year later in bug 1475832 (as a feature request).

I landed this in 77 so if it turns out that I'm mistaken on the use cases, then we can restore the functionality (with less code/complexity than what we had before!) in time before ESR 78 branches off. But there must be a really compelling use case, because the presence of this undocumented feature had very negative side effects (on performance).

== Change summary for alert #25740 (as of Mon, 27 Apr 2020 10:53:46 GMT) ==

Improvements:

1% Base Content JS macosx1014-64-shippable opt 3,725,504.00 -> 3,691,007.33
1% Base Content JS linux1804-64-shippable opt 3,723,252.00 -> 3,688,863.33
1% Base Content JS linux1804-64-shippable-qr opt 3,723,419.33 -> 3,689,060.67
1% Base Content JS windows10-64-shippable-qr opt 3,789,061.33 -> 3,755,483.33
1% Base Content JS windows10-64-shippable opt 3,786,108.00 -> 3,755,554.67
1% Base Content JS windows10-64-shippable-qr opt 3,786,130.67 -> 3,755,619.33
1% Base Content JS windows7-32-shippable opt 2,931,692.00 -> 2,914,714.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=25740

Duplicate of this bug: 1630737

Hi, based on Comment 7 is the conclusion that we don't need any documentation update?

Flags: needinfo?(rob)
Flags: needinfo?(mixedpuppy)

That's right. No documentation update needed, besides possibly an explicit callout that data:-URIs aren't supported.

Only requests made using HTTP or HTTPS will trigger events, even though match patterns can match some other protocols.

Only requests made using HTTP or HTTPS will trigger events, even though match patterns can match some other protocols (such as data: and file:).

Flags: needinfo?(rob)
Flags: needinfo?(mixedpuppy)

I have clarified on webRequest.RequestFilter as follows "Only requests made using HTTP or HTTPS will trigger events, other protocols (such as data: and file:) supported by pattern matching do not trigger events."
Given the nature of the change, also marking as dev doc done, but please let me know if you have any issues or need further changes.

There's the warning about non-HTTP protocols on the main page here - maybe that should get changed or removed? I had added that and it got morphed a bit; I thought it made sense for the limitations on the feature set to be stated front and center to avoid confusion.

Also, I had a question after another reading tonight. Should I expect the HTTP-based (but not http:// protocol handler) ws:// to work? Reading the webRequest page makes me think yes, but reading the requestFilter page leaves me uncertain.

Attached image 2xhJjadfA5.png
Flags: needinfo?(rob)

We should probably add this to https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/77#API_changes , so that there is at least some place where add-on developers can learn from this change.

(In reply to wingman.jr.addon from comment #13)

There's the warning about non-HTTP protocols on the main page here - maybe that should get changed or removed? I had added that and it got morphed a bit; I thought it made sense for the limitations on the feature set to be stated front and center to avoid confusion.

Let's remove that warning from the documentation, as it does not apply.

Also, I had a question after another reading tonight. Should I expect the HTTP-based (but not http:// protocol handler) ws:// to work? Reading the webRequest page makes me think yes, but reading the requestFilter page leaves me uncertain.

WebSocket requests are still supported. We only have very minimal test coverage for it, so we should probably add a test first and then update MDN. I filed bug 1635929 to improve test coverage.

Flags: needinfo?(rob)
You need to log in before you can comment on or make changes to this bug.