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

RESOLVED FIXED in Firefox 58

Status

()

Core
DOM: Service Workers
P2
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: HoPang, Assigned: tt)

Tracking

(Blocks: 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments, 10 obsolete attachments)

667.64 KB, image/png
Details
5.56 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

a year ago
Created attachment 8861733 [details]
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.
(Reporter)

Comment 1

a year ago
I am current busy working on other stuff, after that, I'll put my more detailed thoughts here.
Blocks: 1262699
Priority: -- → P2
(Reporter)

Updated

a year ago
Assignee: nobody → bhsu
(Reporter)

Comment 2

a year ago
Created attachment 8865870 [details] [diff] [review]
(WIP) P1: Introduce ServiceWorkerManager.setBypassServiceWorker()
(Reporter)

Comment 3

a year ago
Created attachment 8865871 [details] [diff] [review]
(WIP) P2: Add a new chrome mochitest for ServiceWorkerManager.setBypassServiceWorker()
(Reporter)

Comment 4

a year ago
Created attachment 8866268 [details] [diff] [review]
P1: Introduce ServiceWorkerManager::SetBypassServiceWorker() and ServiceWorkerManager::GetBypassServiceWorker()
(Reporter)

Comment 5

a year ago
Created attachment 8866271 [details] [diff] [review]
P2: Add a new chrome mochitest
(Reporter)

Updated

a year ago
Attachment #8865870 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8865871 - Attachment is obsolete: true
(Reporter)

Comment 6

11 months ago
Created attachment 8881462 [details] [diff] [review]
P1: Introduce ServiceWorkerManager::SetBypassServiceWorker() and ServiceWorkerManager::GetBypassServiceWorker()
(Reporter)

Updated

11 months ago
Attachment #8866268 - Attachment is obsolete: true
Tom, could you please follow this up?
Assignee: bhsu → nobody
Flags: needinfo?(ttung)
Sure
Assignee: nobody → ttung
Flags: needinfo?(ttung)
Created attachment 8917240 [details] [diff] [review]
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!
Attachment #8881462 - Attachment is obsolete: true
Attachment #8917240 - Flags: review?(bugmail)
Created attachment 8917241 [details] [diff] [review]
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.
Attachment #8866271 - Attachment is obsolete: true
Attachment #8917241 - Flags: review?(bugmail)
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
Attachment #8917240 - Flags: review?(bugmail)
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.
Attachment #8917241 - Flags: review?(bugmail)
(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

8 months ago
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.
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
(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
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!
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko) → needinfo?(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.
Flags: needinfo?(ttung)
(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
Created attachment 8918818 [details] [diff] [review]
Bug 1359677 - P1: Add a new chrome mochitest to verify the bypassing service worker. r?asuth
Attachment #8917240 - Attachment is obsolete: true
Attachment #8917241 - Attachment is obsolete: true
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!
Attachment #8918818 - Flags: review?(bugmail)
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.
Attachment #8918818 - Flags: review?(bugmail) → review-
(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
Created attachment 8920956 [details] [diff] [review]
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.
Attachment #8918818 - Attachment is obsolete: true
Attachment #8920956 - Flags: review?(bugmail)
Created attachment 8920961 [details] [diff] [review]
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.
Attachment #8920956 - Attachment is obsolete: true
Attachment #8920956 - Flags: review?(bugmail)
Attachment #8920961 - Flags: review?(bugmail)
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/
Attachment #8920961 - Flags: review?(bugmail) → review+
(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!
Created attachment 8921291 [details] [diff] [review]
Bug 1359677 - P1: Add a new mochitest to verify the bypassing service worker. r=asuth

Address the comment
Attachment #8920961 - Attachment is obsolete: true
Attachment #8921291 - Flags: review+

Comment 31

7 months ago
Pushed by ttung@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/97db331fc5ac
P1: Add a new mochitest to verify the bypassing service worker. r=asuth
https://hg.mozilla.org/mozilla-central/rev/97db331fc5ac
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Comment 33

7 months ago
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.

Updated

7 months ago
Depends on: 1411746
You need to log in before you can comment on or make changes to this bug.