Closed Bug 1448500 Opened 7 years ago Closed 7 years ago

Proxy API sees speculative connections as "other" connections

Categories

(WebExtensions :: Request Handling, enhancement, P3)

Product:
WebExtensions
Component:
Request Handling
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(1 file)

Bug 1448500: Add speculative request content policy type.
59 bytes, text/x-review-board-request
bzbarsky
: review+
kmag
: review+
Details 
It makes it a little hard to understand what is going on. Everytime you type in the url bar and sometimes while a page is loaded (particularly from the cache I believe) we spin up a speculative connection to a web server. Proxy extensions may want to let the speculative request handler know that a request is eventually going to go through a proxy so I propose at least adding the type="speculative" to the request type, maybe also doing the work to associate it with the correct tabId.
Assignee: nobody → dtownsend
Not totally sure who should review this. kmag for the webextension pieces makes sense. The rest is mostly in DOM so picking bz, but pick an alternate if that doesn't make sense.
Comment on attachment 8963385 [details]
Bug 1448500: Add speculative request content policy type.

https://reviewboard.mozilla.org/r/232288/#review237768

::: browser/components/extensions/test/browser/browser_ext_proxy_speculative.js:47
(Diff revision 1)
> +  await extension.startup();
> +
> +  let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/", true);
> +  await extension.awaitMessage("onUpdated");
> +  await extension.unload();
> +  BrowserTestUtils.removeTab(tab);

I'd generally prefer these kinds of tests to be xpcshell tests. Our browser mochitest runtime is already too long. You can trigger the page load with ExtensionTestUtils.loadContentPage. If that doesn't trigger a speculative connect, we should be able to fix it.

Though I suppose we wouldn't be able to test the tab ID in that case. If we really need to test that here, I'd rather we use a plain mochitest in toolkit/ and use browser.tabs.create or window.open, so we at least get Android and headless support.
Priority: -- → P3
Comment on attachment 8963385 [details]
Bug 1448500: Add speculative request content policy type.

https://reviewboard.mozilla.org/r/232288/#review238094

r=me for the extension parts. Thanks

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_speculative.js:13
(Diff revision 3)
> +    browser.proxy.onRequest.addListener(details => {
> +      browser.test.log(`onRequest ${JSON.stringify(details)}`);
> +      browser.test.assertEq(details.type, "speculative", "Should have seen a speculative proxy request.");
> +      return [{type: "direct"}];
> +    }, {urls: ["<all_urls>"]}, ["requestHeaders"]);
> +    browser.test.sendMessage("ready");

No need for this. startup() doesn't resolve until the background script finishes executing. This kind of thing is only necessary if there are `await`s and such.

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_speculative.js:31
(Diff revision 3)
> +  Services.prefs.setBoolPref("network.http.debug-observations", true);
> +
> +  await handlingExt.startup();
> +  await handlingExt.awaitMessage("ready");
> +
> +  let notificationPromise = new Promise(resolve => {

Nit: `ExtensionUtils.promiseObserved("speculative-connect-request");`

::: toolkit/components/extensions/test/xpcshell/test_ext_proxy_speculative.js:35
(Diff revision 3)
> +
> +  let notificationPromise = new Promise(resolve => {
> +    Services.obs.addObserver(resolve, "speculative-connect-request");
> +  });
> +
> +  let uri = NetUtil.newURI(`http://${proxy.identity.primaryHost}:${proxy.identity.primaryPort}`);

Nit: s/NetUtil/Services.io/. I think florian is trying to get rid of NetUtil.newURI
Attachment #8963385 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8963385 [details]
Bug 1448500: Add speculative request content policy type.

https://reviewboard.mozilla.org/r/232288/#review238244

::: dom/base/nsContentPolicyUtils.h:117
(Diff revision 4)
>      CASE_RETURN( TYPE_XSLT                        );
>      CASE_RETURN( TYPE_BEACON                      );
>      CASE_RETURN( TYPE_FETCH                       );
>      CASE_RETURN( TYPE_IMAGESET                    );
>      CASE_RETURN( TYPE_WEB_MANIFEST                );
>      CASE_RETURN( TYPE_SAVEAS_DOWNLOAD             );

Should probably have the entries in numerical order here, right?

::: dom/base/nsIContentPolicy.idl:190
(Diff revision 4)
>     * Indicates an save-as link download from the front-end code.
>     */
>    const nsContentPolicyType TYPE_SAVEAS_DOWNLOAD = 43;
>  
>    /**
> +   * Indicates a speculative connection.

This, and the existing "43" should go at the end of the list, no?

::: dom/base/nsIContentPolicy.idl:353
(Diff revision 4)
>    const nsContentPolicyType TYPE_INTERNAL_WORKER_IMPORT_SCRIPTS = 42;
>  
>    /* When adding new content types, please update nsContentBlocker,
>     * NS_CP_ContentTypeName, nsCSPContext, all nsIContentPolicy
>     * implementations, the static_assert in dom/cache/DBSchema.cpp,
> -   * ChannelWrapper.webidl, ChannelWrapper.cpp, and other things that
> +   * ChannelWrapper.webidl, ChannelWrapper.cpp, nsPermissionManager.cpp, and

Should mention nsCSPUtils.cpp CSP_ContentTypeToDirective here.  And nsContentSecurityManager.cpp (DoContentSecurityChecks).

You're going to need to merge a bit on top of bug 1439713.

Do we need to add this type (and TYPE_SAVEAS_DOWNLOAD) to kTypeString in extensions/permissions/nsContentBlocker.cpp?

::: dom/cache/DBSchema.cpp:314
(Diff revision 4)
>                nsIContentPolicy::TYPE_XSLT == 18 &&
>                nsIContentPolicy::TYPE_BEACON == 19 &&
>                nsIContentPolicy::TYPE_FETCH == 20 &&
>                nsIContentPolicy::TYPE_IMAGESET == 21 &&
>                nsIContentPolicy::TYPE_WEB_MANIFEST == 22 &&
>                nsIContentPolicy::TYPE_SAVEAS_DOWNLOAD == 43 &&

I really wonder why SAVEAS_DOWNLOAD was randomly added in the middle somewhere.  Please move these to the end.
Attachment #8963385 - Flags: review?(bzbarsky) → review+
Christoph, Dave pointed out on IRC that maybe the ordering (43 between 22 and 23) is so that all the non-internal things come first.  Is that what's going on here?

If so, we should probably add some comments explaining that, and a comment at the end of the list saying that the largest numeric thing might be somewhere in the middle (and where) or something....
Flags: needinfo?(ckerschb)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> Christoph, Dave pointed out on IRC that maybe the ordering (43 between 22
> and 23) is so that all the non-internal things come first.  Is that what's
> going on here?

Probably baku added it there with that intention, but in fact I should have complained about that when reviewing the patch - apparently I haven't realized it. Can you please move TYPE_SAVEAS_DOWNLOAD at the end of the list? Thanks for fixing and sorry about that!
Flags: needinfo?(ckerschb)
Pushed by dtownsend@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5f6843bb63
Add speculative request content policy type. r=bz, r=kmag
https://hg.mozilla.org/mozilla-central/rev/ae5f6843bb63
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(dtownsend)
Flags: needinfo?(dtownsend) → qe-verify-
Product: Toolkit → WebExtensions
Depends on: 1479565
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: