Closed Bug 1254204 Opened 4 years ago Closed 3 years ago

Support a suspend/resume mechanism in WebRequest

Categories

(WebExtensions :: Request Handling, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla52
Iteration:
48.1 - Mar 21

People

(Reporter: mao, Assigned: kmag)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [webRequest] triaged)

Attachments

(3 files)

In order to make anything useful with big requestBody instances (Bug 1201979), as specifically seen on the field with NoScript's XSS filter which analyzes POST payloads for possible XSS attacks, we need a way to do suspend the underlying channel while we're working asynchronously and possibly resume or cancel it when we're done, otherwise we're gonna stall the parent process in an unacceptable way.

What I propose, now that we've got requestId implemented, is supporting a

{suspended: true}

return value from "blocking" listeners (similar to "cancel" or "redirect") which prompts our implementation to invoke channel.suspend() and store a *weak* reference to the channel in a Map, keyed by its requestId.

In order to resume the request, we could implement a

chrome.webRequest.resume(requestId)

method, attempts to retrieve the previously suspended channel (if still referenced somewhere else, i.e. not dead with an already closed browsing context) and resume it, returning true if the channel has been successfully retrieved and resumed, false otherwise.
Bill, Kris, as discussed minutes ago I'd like some feedback on my plan. I'm of course open to different implementation strategies / APIs, as far as we manage to act on the webRequest lifecycle even if our decision depend on asynchronous, potentially long-running tasks.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(kmaglione+bmo)
Once we have out-of-process WebExtensions, I think we're going to suspend/resume for every request when the "blocking" option is present. We won't be able to make blocking decisions synchronously since communication with the extension process will be asynchronous. Therefore I think we'll have to suspend the channel whenever a WebExtension needs to make a blocking/redirect decision.

I think it would be cool to try to do this now even without OOP WebExtensions. We would always do a suspend operation if the OnBeforeRequestOptions contains "blocking". Then we would asynchronously invoke the webRequest onBeforeRequest callback and resume the channel when it finished (unless we cancel). This will almost certainly have an effect on performance, but I don't see any way around it if we want OOP extensions. Doing it now will allow us to explore the design and will make it easier to implement OOP extensions.

Another thing that would be kind of cool would be to allow the onBeforeRequest to return a promise. That way the WebExtension could make its decision asynchronously. I know Chrome has always avoided this because they're afraid that extensions could block requests forever. But I think it's probably inevitable for us.

Kris, does this seem like a reasonable plan to you?
Flags: needinfo?(wmccloskey)
Yeah, that sounds good to me. I definitely like the idea of returning a promise to delay resuming the channel.

The only real concern I have is how we handle onBeforeSendHeaders events. They need to be executed sequentially, so header changes from one extension don't blindly overwrite header changes from another. If we have multiple extensions listening on that event, and they all need to do asynchronous work, ideally they should be able to do it in parallel. But if we go with something this simple, they won't be able to.

Maybe that's a corner case we don't need to worry about, or maybe we should only support suspending the channel from onBeforeRequest, but we should probably try to figure it out before we start implementing it.
Flags: needinfo?(kmaglione+bmo)
That's a good point. However, my reading of the docs [1] is that a given header can't be modified by more than one extension. It's a little unclear what the phrase "at a time" means though.

Anyway, that suggests that we could run them in parallel and then merge the results on a per-header basis. We'd have to compare against the original value of the header, but we do that anyway I think.

[1] https://developer.chrome.com/extensions/webRequest#implementation (specifically on "Conflict resolution")
I think that makes sense.

In that case, I think we should move forward with this plan, if it sounds good to Giorgio.
(In reply to Kris Maglione [:kmag] from comment #5)
> I think that makes sense.
> 
> In that case, I think we should move forward with this plan, if it sounds
> good to Giorgio.

Yes. I bet you use this line a lot around there, but the Promise approach sounds promising, and it surely will keep the API much cleaner.
Should I take the bug?
(In reply to Giorgio Maone from comment #6)
> (In reply to Kris Maglione [:kmag] from comment #5)
> > I think that makes sense.
> > 
> > In that case, I think we should move forward with this plan, if it sounds
> > good to Giorgio.
> 
> I bet you use this line a lot around there, but the Promise approach
> sounds promising

Heh. No, I think this may be the first time I've heard it...

> Should I take the bug?

Please do :)
Assignee: nobody → g.maone
Blocks: 1255894
> {suspended: true}
> chrome.webRequest.resume(requestId)

and please add:
chrome.webRequest.cancel(requestId)
No longer blocks: 1201979
Priority: -- → P1
Whiteboard: [webRequest] triaged
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Sorry, stealing this. It's blocking some OOP work I need to do.
Assignee: g.maone → kmaglione+bmo
Blocks: 1305217
Attachment #8808913 - Flags: review?(aswan) → review?(mixedpuppy)
Attachment #8808914 - Flags: review?(aswan) → review?(mixedpuppy)
Attachment #8808915 - Flags: review?(aswan) → review?(mixedpuppy)
Comment on attachment 8808915 [details]
Bug 1254204: Part 3 - Refactor runChennelListener to decrease cyclomatic complexity.

https://reviewboard.mozilla.org/r/91634/#review91684
Attachment #8808915 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8808913 [details]
Bug 1254204: Part 1 - Apply WebRequest header changes differentially, after all listeners have run in parallel.

https://reviewboard.mozilla.org/r/91630/#review91708

::: toolkit/modules/addons/WebRequest.jsm:158
(Diff revision 1)
> +
> +  applyChanges(headers) {
> +    if (!this.validateHeaders(headers)) {
> +      /* globals uneval */
> +      Cu.reportError(`Invalid header array: ${uneval(headers)}`);
> +      dump(`Invalid header array: ${uneval(headers)}\n`);

remove dump

::: toolkit/modules/addons/WebRequest.jsm:189
(Diff revision 1)
> +class RequestHeaderChanger extends HeaderChanger {
> +  setHeader(name, value) {
> +    try {
> +      this.channel.setRequestHeader(name, value, false);
> +    } catch (e) {
> +      dump(`Error setting request header ${name}: ${e}\n`);

remove dump

::: toolkit/modules/addons/WebRequest.jsm:214
(Diff revision 1)
> +        getData(this.channel).contentType = value;
> +      } else {
> +        this.channel.setResponseHeader(name, value, false);
> +      }
> +    } catch (e) {
> +      dump(`Error setting response header ${name}: ${e}\n`);

dump dump dump

::: toolkit/modules/addons/WebRequest.jsm:592
(Diff revision 1)
> +    let requestHeaders = new RequestHeaderChanger(channel);
> +    let responseHeaders;
> +    try {
> +      responseHeaders = new ResponseHeaderChanger(channel);
> +    } catch (e) {
> +      // Meh.

better comment

::: toolkit/modules/addons/WebRequest.jsm:689
(Diff revision 1)
>          channel.cancel(Cr.NS_ERROR_ABORT);
>          this.errorCheck(channel, loadContext);
>          return false;
>        }
> +
> +      // FIXME: This should only be available in some phases.

Fix now or followup bug#?
Attachment #8808913 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8808913 [details]
Bug 1254204: Part 1 - Apply WebRequest header changes differentially, after all listeners have run in parallel.

https://reviewboard.mozilla.org/r/91630/#review91708

> remove dump

Oops. Yeah, I meant to remove them. For some reason, the reportError calls weren't showing up in mochitest output :/

> Fix now or followup bug#?

I think that this is actually handled by the schema validation of the options object, so I should probably just remove the comment.
Comment on attachment 8808914 [details]
Bug 1254204: Part 2 - Allow suspending requests by returning Promises from blocking request listeners.

https://reviewboard.mozilla.org/r/91632/#review91692

::: toolkit/components/extensions/test/mochitest/return_headers.sjs:9
(Diff revision 1)
> +
> +function handleRequest(request, response) {
> +  response.setHeader("Content-Type", "text/plain", false);
> +
> +  let headers = {};
> +  // Why on earth...

potentially confusing comment, expand or remove?

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html:255
(Diff revision 1)
>  
>      let headers = details[`${phase}Headers`];
>      browser.test.assertTrue(Array.isArray(headers), `valid ${phase}Headers array`);
>  
>      let {added, modified, deleted} = testHeaders[phase];
> +    browser.test.log(`... ${uneval(added)} ${uneval(headers)}`);

Is this necessary to keep?

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_suspend.html:44
(Diff revision 1)
> +
> +  yield extension.startup();
> +
> +  let result = yield fetch(SimpleTest.getTestFileURL("return_headers.sjs"));
> +
> +  let headers = JSON.parse(yield result.text());

Is there any benefit to also examining response headers here?

::: toolkit/modules/addons/WebRequest.jsm:751
(Diff revision 1)
> -    let loadContext = this.getLoadContext(channel);
>  
> -    if (this.runChannelListener(channel, loadContext, "opening") &&
> -        this.runChannelListener(channel, loadContext, "modify")) {
> -      this.runChannelListener(channel, loadContext, "afterModify");
> +    if (kind === "opening") {
> +      return this.runChannelListener(channel, loadContext, "modify");
> +    } else if (kind === "modify") {
> +      return this.runChannelListener(channel, loadContext, "afterModify");

what are we returning here?
Attachment #8808914 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8808914 [details]
Bug 1254204: Part 2 - Allow suspending requests by returning Promises from blocking request listeners.

https://reviewboard.mozilla.org/r/91632/#review91692

> potentially confusing comment, expand or remove?

Why on earth is the request.headers property implemented as a nsISimpleEnumerator of nsISupportsStrings objects when it's implemented completely in JS and isn't used by any native code?

> Is this necessary to keep?

Nope, I meant to remove it. But, if I'm honest, it will probably be handy the next time these tests explode...

> Is there any benefit to also examining response headers here?

Probably not. None of these APIs have an effect on the headers the server sends back to us, and they're already predictable. The request headers checks are necessary to make sure that suspending the request before the headers are sent works, and we actually wind up sending the right headers.

Testing that suspending works for other blocking handlers would be good too, but it will happen by default once bug 1305217 is done.

> what are we returning here?

Nothing, at the moment, but in the future potentially a promise if runChannelListener starts returning the result of applyChanges.
https://hg.mozilla.org/integration/mozilla-inbound/rev/30aa827dc7849d11478f2bcc821825c324f41de8
Bug 1254204: Part 1 - Apply WebRequest header changes differentially, after all listeners have run in parallel. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/07b39b68b0844b18d16747dd9a9502d47d2c487a
Bug 1254204: Part 2 - Allow suspending requests by returning Promises from blocking request listeners. r=mixedpuppy

https://hg.mozilla.org/integration/mozilla-inbound/rev/b913f0ce1e3e3bc94ce444b286768aed15efbfd4
Bug 1254204: Part 3 - Refactor runChennelListener to decrease cyclomatic complexity. r=mixedpuppy
Keywords: dev-doc-needed
I've noted this here:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest#Modifying_requests
and in the preamble to each of the APIs this affects:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onBeforeRequest
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onBeforeSendHeaders
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onHeadersReceived

I've also added examples showing async listeners to those pages. They're quite silly examples (just resolving the promise in a timer) but they should illustrate the idea (unless I've misunderstood it).

Let me know if this looks right to you.
Flags: needinfo?(kmaglione+bmo)
Product: Toolkit → WebExtensions
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.