1359677 - Implement a new internal platform API for "bypass for network"

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P2)

Description

[Ben Hsu [:HoPang]]

Attachment: Screenshot from 2017-04-26 11-13-56.png

Chrome devtools have a sweet little button to make newly created outgoing network request (such as fetch(...)) always bypass service workers. This feature is proved useful, so let's do it.

BTW, in the attachment, you can find a cute little checkbox highlighted by an orange block which toggle this feature.

Comment 1

[Ben Hsu [:HoPang]]

I am current busy working on other stuff, after that, I'll put my more detailed thoughts here.

Comment 2

[Ben Hsu [:HoPang]]

Attachment: (WIP) P1: Introduce ServiceWorkerManager.setBypassServiceWorker()

Comment 3

[Ben Hsu [:HoPang]]

Attachment: (WIP) P2: Add a new chrome mochitest for ServiceWorkerManager.setBypassServiceWorker()

Comment 4

[Ben Hsu [:HoPang]]

Attachment: P1: Introduce ServiceWorkerManager::SetBypassServiceWorker() and ServiceWorkerManager::GetBypassServiceWorker()

Comment 5

[Ben Hsu [:HoPang]]

Attachment: P2: Add a new chrome mochitest

Comment 6

[Ben Hsu [:HoPang]]

Attachment: P1: Introduce ServiceWorkerManager::SetBypassServiceWorker() and ServiceWorkerManager::GetBypassServiceWorker()

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Tom, could you please follow this up?

Comment 8

[Tom Tung [:tt, :ttung]]

Sure

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1359677 - P1: Introduce ServiceWorkerManager::SetBypassServiceWorker() and ServiceWorkerManager::GetBypassServiceWorker(). r?asuth

hopang's patches look fine to me, so I only rebase his patches and send them to review.

Hi Andrew,

This patch is mainly to introduce two internal APIs to make our dev-tools be able to have an option to bypass the service worker per tab in the future (We only have bypass network for now). Would you mind helping me to review it when you have time? Thanks!

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1359677 - P2: Add a new chrome mochitest to verify bypass service worker API. r?asuth

This patch is mainly to have a test to verify two new APIs.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8917240 [details] [diff] [review]
Bug 1359677 - P1: Introduce ServiceWorkerManager::SetBypassServiceWorker() and ServiceWorkerManager::GetBypassServiceWorker(). r?asuth

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

So, this seems like it works, but I think we want to imitate the "Disable cache" option or have an explicit reason for avoiding it.  An upside of this would be to avoid this additional machinery.

"Disable cache" operates by setting the docshell's defaultLoadFlags to `Ci.nsIRequest.LOAD_BYPASS_CACHE | Ci.nsIRequest.INHIBIT_CACHING`[1].

We have nsIChannel::LOAD_BYPASS_SERVICE_WORKER, and although in places it's used in a more internal fashion, such as avoiding re-intercepting after we reset interception, we also use it semantically as part of our "don't use SW on shift-reload logic"[2].

Given :bkelly's recent work in this area, he'll probably have some good ideas on this at our meeting.

1: https://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/devtools/server/actors/tab.js#1074
2: https://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/docshell/base/nsDocShell.cpp#11712

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8917241 [details] [diff] [review]
Bug 1359677 - P2: Add a new chrome mochitest to verify bypass service worker API. r?asuth

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

This test really wants to be a "browser" (chrome) test rather than a "chrome" test.  In particular, it's not currently e10s friendly.  (Also, the .then() chains really want to be re-written as async functions if kept around.)

Bug 1047663 which implemented the "disable cache" support for (DOM)Workers is along the lines of what we want, although I think we have modern helpers that make things easier now, wrapping the calls to loadFrameScript, etc.  Specifically, something like the following can be used to set the flag:

  await ContentTask.spawn(
    tab.linkedBrowser, null,
    function() {
      docShell.defaultLoadFlags |= Components.interfaces.nsIChannel.LOAD_BYPASS_SERVICE_WORKER;
    });

Once the flag is set, the tab can be navigated via:
  await BrowserTestUtils.loadURI(tab.linkedBrowser, url));
  await BrowserTestUtils.browserLoaded(tab.linkedBrowser);

The page can be directly inspected and/or its JS invoked by using ContentTask.spawn() (using `content.wrappedJSObject` to access content-defined JS), or events waited for via BrowserTestUtils.waitForEvent(), etc.

Comment 13

[Tom Tung [:tt, :ttung]]

(In reply to Andrew Sutherland [:asuth] from comment #11)
Thanks for the feedback and explanation! I'll reimplement it.

> Comment on attachment 8917240 [details] [diff] [review]
> Bug 1359677 - P1: Introduce ServiceWorkerManager::SetBypassServiceWorker()
> and ServiceWorkerManager::GetBypassServiceWorker(). r?asuth
> 
> Review of attachment 8917240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, this seems like it works, but I think we want to imitate the "Disable
> cache" option or have an explicit reason for avoiding it.  An upside of this
> would be to avoid this additional machinery.

I don't have any strong opinion for not imitating the "Disable cache" for now, and I agree that we should avoid introducing extra machinery. So, I'll change this implementation to modify the docshell's defaultLoadFlags. 

I guess I also need to change the code for enable/disable cache [1] to make it don't get rid out other flag (given that LOAD_BYPASS_SERVICE_WORKER might be set) and don't reset the flag to Ci.nsIRequest.LOAD_NORMAL.

> Given :bkelly's recent work in this area, he'll probably have some good
> ideas on this at our meeting.

I'll ask for his feedback about passing LOAD_BYPASS_SERVICE_WORKER to defaultLoadFlag.

[1] https://searchfox.org/mozilla-central/source/devtools/server/actors/tab.js#1077

(In reply to Andrew Sutherland [:asuth] from comment #12)
> Comment on attachment 8917241 [details] [diff] [review]
> Bug 1359677 - P2: Add a new chrome mochitest to verify bypass service worker
> API. r?asuth
> 
> Review of attachment 8917241 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This test really wants to be a "browser" (chrome) test rather than a
> "chrome" test.  In particular, it's not currently e10s friendly.  (Also, the
> .then() chains really want to be re-written as async functions if kept
> around.)

Yes, it should be written as a browser chrome test. I'll rewrite it. Thanks!

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

The stuff docshell does tries to prevent navigator.serviceWorker.controller to be set for the client.  The chrome checkbox, though, just bypasses the SW on network request and allows the page to still be controlled.  So I think you just want the nsIChannel LOAD_BYPASS_SERVICE_WORKER flag.

Comment 15

[Tom Tung [:tt, :ttung]]

On service worker meeting, the suggestions are that:
- we should see what chrome's behavior
- we should cc (or even ask feedback?) from devtools people for UX and how they think it should work

Comment 16

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #15)
> - we should see what chrome's behavior
I guess I misunderstanding this per 14.

(In reply to Ben Kelly [:bkelly] from comment #14)
> The stuff docshell does tries to prevent navigator.serviceWorker.controller
> to be set for the client.  The chrome checkbox, though, just bypasses the SW
> on network request and allows the page to still be controlled.  So I think
> you just want the nsIChannel LOAD_BYPASS_SERVICE_WORKER flag.

Thanks! I'll try to do that

Comment 17

[Tom Tung [:tt, :ttung]]

Hi Honza,

We are planning to have an internal API to allow dev-tools to bypass the service worker for a tab (top level window/document). So that in the future, we could have an option/checkbox to enable/disable service worker for a tab just like "Diable cache" option.

We could do this just like the chrome did, which only bypasses the SW on network request and allows the page to still be controlled) or we could do something even more (maybe stop the page to be controlled as well?).

Would you mind giving us some more inputs here? Thanks!

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Tom Tung [:tt] from comment #17)
> We are planning to have an internal API to allow dev-tools to bypass the
> service worker for a tab (top level window/document). So that in the future,
> we could have an option/checkbox to enable/disable service worker for a tab
> just like "Diable cache" option.
Yes, sounds great

> We could do this just like the chrome did, which only bypasses the SW on
> network request and allows the page to still be controlled) or we could do
> something even more (maybe stop the page to be controlled as well?).
It would be great if we can have more (to attract users) while keeping the necessary UI simple.
Do you have any suggestion of what specific features the user could get?
What do you mean by 'controlling the page'? E.g. push notifications?

Honza

Comment 19

[Tom Tung [:tt, :ttung]]

(In reply to Jan Honza Odvarko [:Honza] from comment #18)
Thanks for the feedback!

> It would be great if we can have more (to attract users) while keeping the
> necessary UI simple.
> Do you have any suggestion of what specific features the user could get?

Honestly, I don't have any idea for now. I think we could just have a basic API just as chrome did. I'll keep thinking about what features can we provide to developers.

> What do you mean by 'controlling the page'? E.g. push notifications?

I think you are right. Just like we don't have the service worker. Also, we cannot use the console to get the service worker by navigator.serviceWorker.controller in that window. However, I just find out the developers could do the same thing if they terminate the service worker.

Anyway, I'll have basic API just as chrome did first.

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Tom Tung [:tt] from comment #19)
> Anyway, I'll have basic API just as chrome did first.
Yes, make sense to me.

Honza

Comment 21

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1359677 - P1: Add a new chrome mochitest to verify the bypassing service worker. r?asuth

Comment 22

[Tom Tung [:tt, :ttung]]

Comment on attachment 8918818 [details] [diff] [review]
Bug 1359677 - P1: Add a new chrome mochitest to verify the bypassing service worker. r?asuth

Hi Andrew,

Because the implementation of nsIChannel::BYPASS_SERVICE_WORKER has been done, I only write a browser test to ensure that we can disable a service worker for hijacking requests from a document by setting the nsIChannel::BYPASS_SERVICE_WORKER to defaultLoadFlags in the docShell. So that, in the future, we can implement this feature to dev-tools easily by following the code for enabling/disable an SW for a document.

Could you help me to review it? Thanks!

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8918818 [details] [diff] [review]
Bug 1359677 - P1: Add a new chrome mochitest to verify the bypassing service worker. r?asuth

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

In retrospect, I think I probably should have suggested to just write this as a plain mochitest if we're not actually trying to trigger devtools or use an nsIServiceWorkerManager-exposed API.  (Apologies!)  SpecialPowers can provide access to iframe like so:
  SpecialPowers.wrap(someIframe.contentWindow).QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebNavigation).QueryInterface(Ci.nsIDocShell);
I think the current structuring works though; either way is fine with me.  (I definitely fine SpecialPowers.wrap(x) a little weird, at least! :)

Test-wise, I think we want also want to check and assert that the document in question is controlled.  As I read the test, it currently does the following:
- Loads blank.html into the top-level content window.
- Creates an about:blank iframe beneath that window.
- Registers the fetch.js with the default scope (which is "./" relative to the "fetch.js" SW script, so the current directory) and waits for the register call to resolve which guarantees that (assuming this isn't a resurrection) the SW is in the installing state.
- (Does not send a "claim" message to fetch.js so it doesn't claim any clients.)
- Sets LOAD_BYPASS_SERVICE_WORKER on the top-level content window's docshell.  But since I don't believe the doc is controlled, I'm not sure this does anything useful.
- Does some fetches in the top-level content window with bypassing enabled and disabled, but again, I don't think the doc gets controlled.

test_fetch_integrity.html which this test looks a little bit based on in terms of helpers it uses sortof has logic along these lines already in test_integrity_serviceWorker().  Specifically, it has logic to wait for itself to become controlled, then issues some fetches.  It might make sense to re-derive the test logic here from that test.  The key thing is to make sure our document is controlled.  (And I think it's preferable to avoid involving about:blank iframes in tests unless we're explicitly testing edge-cases related to origin/controller inheritance.)

::: dom/workers/test/serviceworkers/browser_devtools_bypass_serviceworker.js
@@ +12,5 @@
> +  // Bypass SW imitates the "Disable Cache" option in dev-tools.
> +  // Note: if we put the setter/getter into dev-tools, we should take care of
> +  // the implementation of enabling/disabling cache since it just overwrite the
> +  // defaultLoadFlags of docShell.
> +  function setBypassServiceWorker(aDocShell, aDisable) {

nit: I'd rename aDisable here to aBypass.  Naming the argument disable makes me think we might be disabling the bypass, which is clearly not the intent.

@@ +29,5 @@
> +
> +  async function fetchAndCheckIfIntercepted(aWindow, aFakeDoc) {
> +    let response = await aWindow.fetch(aFakeDoc)
> +    let text = await response.text();
> +    if (text.indexOf("Hello") !== -1) {

fyi, no need to fix here, but you can do text.includes("Hello") since Firefox 40 which I find to be more readable.

@@ +31,5 @@
> +    let response = await aWindow.fetch(aFakeDoc)
> +    let text = await response.text();
> +    if (text.indexOf("Hello") !== -1) {
> +      // Intercepted
> +      return Promise.resolve(true);

You can just return true directly here.

@@ +34,5 @@
> +      // Intercepted
> +      return Promise.resolve(true);
> +    } else if (text.indexOf("Fake") !== -1) {
> +      // Original page
> +      return Promise.resolve(false);

(and return false here)

@@ +37,5 @@
> +      // Original page
> +      return Promise.resolve(false);
> +    }
> +
> +    return Promise.reject("Response not found");

You can just do `throw "Response not found";` here.

Comment 24

[Tom Tung [:tt, :ttung]]

(In reply to Andrew Sutherland [:asuth] from comment #23)

Thanks for the feedback and the hints! I should notice that I had given up the original implementation on SWM and I didn't check anything on chrome process on the test, so there is no reason to have a browser test rather than a mochitest.

> Comment on attachment 8918818 [details] [diff] [review]
> Bug 1359677 - P1: Add a new chrome mochitest to verify the bypassing service
> worker. r?asuth
> 
> Review of attachment 8918818 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In retrospect, I think I probably should have suggested to just write this
> as a plain mochitest if we're not actually trying to trigger devtools or use
> an nsIServiceWorkerManager-exposed API.  (Apologies!)  SpecialPowers can
> provide access to iframe like so:
>  
> SpecialPowers.wrap(someIframe.contentWindow).QueryInterface(Ci.
> nsIInterfaceRequestor).getInterface(Ci.nsIWebNavigation).QueryInterface(Ci.
> nsIDocShell);
> I think the current structuring works though; either way is fine with me. 
> (I definitely fine SpecialPowers.wrap(x) a little weird, at least! :)
> 
> Test-wise, I think we want also want to check and assert that the document
> in question is controlled.  As I read the test, it currently does the
> following:
> - Loads blank.html into the top-level content window.
> - Creates an about:blank iframe beneath that window.
> - Registers the fetch.js with the default scope (which is "./" relative to
> the "fetch.js" SW script, so the current directory) and waits for the
> register call to resolve which guarantees that (assuming this isn't a
> resurrection) the SW is in the installing state.
> - (Does not send a "claim" message to fetch.js so it doesn't claim any
> clients.)
> - Sets LOAD_BYPASS_SERVICE_WORKER on the top-level content window's
> docshell.  But since I don't believe the doc is controlled, I'm not sure
> this does anything useful.
> - Does some fetches in the top-level content window with bypassing enabled
> and disabled, but again, I don't think the doc gets controlled.
> 
> test_fetch_integrity.html which this test looks a little bit based on in
> terms of helpers it uses sortof has logic along these lines already in
> test_integrity_serviceWorker().  Specifically, it has logic to wait for
> itself to become controlled, then issues some fetches.  It might make sense
> to re-derive the test logic here from that test.  The key thing is to make
> sure our document is controlled.  (And I think it's preferable to avoid
> involving about:blank iframes in tests unless we're explicitly testing
> edge-cases related to origin/controller inheritance.)

I see. I should wait for the service worker's client to be controlled. Otherwise it might be a intermitent failure.

> ::: dom/workers/test/serviceworkers/browser_devtools_bypass_serviceworker.js
> @@ +12,5 @@
> > +  // Bypass SW imitates the "Disable Cache" option in dev-tools.
> > +  // Note: if we put the setter/getter into dev-tools, we should take care of
> > +  // the implementation of enabling/disabling cache since it just overwrite the
> > +  // defaultLoadFlags of docShell.
> > +  function setBypassServiceWorker(aDocShell, aDisable) {
> 
> nit: I'd rename aDisable here to aBypass.  Naming the argument disable makes
> me think we might be disabling the bypass, which is clearly not the intent.

Will do

> @@ +29,5 @@
> > +
> > +  async function fetchAndCheckIfIntercepted(aWindow, aFakeDoc) {
> > +    let response = await aWindow.fetch(aFakeDoc)
> > +    let text = await response.text();
> > +    if (text.indexOf("Hello") !== -1) {
> 
> fyi, no need to fix here, but you can do text.includes("Hello") since
> Firefox 40 which I find to be more readable.

Sure, I'll replace "indexedOf()" to "includes"

> @@ +31,5 @@
> > +    let response = await aWindow.fetch(aFakeDoc)
> > +    let text = await response.text();
> > +    if (text.indexOf("Hello") !== -1) {
> > +      // Intercepted
> > +      return Promise.resolve(true);
> 
> You can just return true directly here.

Will do

> @@ +34,5 @@
> > +      // Intercepted
> > +      return Promise.resolve(true);
> > +    } else if (text.indexOf("Fake") !== -1) {
> > +      // Original page
> > +      return Promise.resolve(false);
> 
> (and return false here)

Will do 

> @@ +37,5 @@
> > +      // Original page
> > +      return Promise.resolve(false);
> > +    }
> > +
> > +    return Promise.reject("Response not found");
> 
> You can just do `throw "Response not found";` here.

Will do

Comment 25

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1359677 - P1 (v2): Add a new mochitest to verify the bypassing service worker. r?asuth

Hi Andrew,

I rewrote the test to a plain mochitest and addressed the comment. Besides, rather than creating a blank iframe to register a service worker, I using the main frame to register a service worker and force itself to be controlled by "claim". Also, I removed the "fake.html", so the testing logic changes a little (`response.status === 404` means not be intercepted).

The rest of logic is the same.

Comment 26

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1359677 - P1 (v2): Add a new mochitest to verify the bypassing service worker. r?asuth

Sorry, I updated a wrong patch. This one is the latest version.

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8920961 [details] [diff] [review]
Bug 1359677 - P1 (v2): Add a new mochitest to verify the bypassing service worker. r?asuth

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

Fantastic, thank you for overhauling the test into a mochitest.  (And this is a great time for us to be adding this coverage given the changes in interception that have recently landed and will land soon!)

::: dom/workers/test/serviceworkers/test_devtools_bypass_serviceworker.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title> Verify devtools can utilize nsIChannel::BYPASS_SERVICE_WORKER to bypass the service worker </title>

nit: s/BYPASS_SERVICE_WORKER/LOAD_BYPASS_SERVICE_WORKER/.  Also in the commit message too.

@@ +68,5 @@
> +     "The loadFlags in docShell does bypass the serviceWorker by default");
> +
> +  let intercepted = await fetchFakeDocAndCheckIfIntercepted(window);
> +  ok(!intercepted,
> +     "The fetched document doesn't be intercepted by the serviceWorker");

phrasing nit: s/doesn't be/wasn't/.

@@ +80,5 @@
> +
> +  intercepted = await fetchFakeDocAndCheckIfIntercepted(window);
> +
> +  ok(intercepted,
> +     "The fetched document does be intercepted by the serviceWorker");

phrasing nit: s/does be/was/

@@ +96,5 @@
> +add_task(async function test_bypassServiceWorker() {
> +  const swURL = "fetch.js";
> +  let registration = await navigator.serviceWorker.register(swURL);
> +
> +  // Wait for the service worker to controll the document

typo nit: s/controll/control/

Comment 28

[Tom Tung [:tt, :ttung]]

(In reply to Andrew Sutherland [:asuth] from comment #27)
> Fantastic, thank you for overhauling the test into a mochitest.  (And this
> is a great time for us to be adding this coverage given the changes in
> interception that have recently landed and will land soon!)

Thanks for the review and giving feedbacks! I learned a lot from them!

Comment 29

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1359677 - P1: Add a new mochitest to verify the bypassing service worker. r=asuth

Address the comment

Comment 30

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=666de884664d94baf7335808c08a0fcbbec35e42

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97db331fc5ac
P1: Add a new mochitest to verify the bypassing service worker. r=asuth

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/97db331fc5ac

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8921291 [details] [diff] [review]
Bug 1359677 - P1: Add a new mochitest to verify the bypassing service worker. r=asuth

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

::: dom/workers/test/serviceworkers/test_devtools_bypass_serviceworker.html
@@ +100,5 @@
> +  });
> +
> +  let sw =
> +    registration.active || registration.waiting || registration.installing;
> +  sw.postMessage("claim");

There is a race condition here that exists in some other tests.  You can't call clients.claim() if the SW is not in the activating or activated states.  I'll file a follow-up bug to fix this here and in other tests.