Jordan Santell [:jsantell] [@jsantell] Reporter
	Description • 9 years ago

Running into issues with new tests I write when running after this test. Suspect it may be because the service worker is never unregistered? And the tests touch the content directly, which all of this should be done with frame scripts so this runs on e10s.

This will be disabled in bug 1134778.

	Alexandre Poirot [:ochameau]
	Comment 1 • 9 years ago

The test may be in better shape with bug 1161072 first patch. It doesn't stop using content, but should help cleaning up state at the end of this test.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 2 • 9 years ago

/r/8583 - Bug 1161072 - Reset docshell state (disabled js/cache, service workers) from actor instead of client. r=jryans
/r/8585 - Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau

Pull down these commits:

hg pull -r 0cef427f22d7335d53e52e8dd79d248a99f5364a https://reviewboard-hg.mozilla.org/gecko/

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 3 • 9 years ago

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #0)
> Running into issues with new tests I write when running after this test.
> Suspect it may be because the service worker is never unregistered? And the
> tests touch the content directly, which all of this should be done with
> frame scripts so this runs on e10s.

Jordan, I stared working on this and used the frame script thing as you could see in the mozreview attached (the frame script thing is new for me). Could please elaborate a bit more who the e10s support could be added with this frame script mechanism please? Thanks!

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 4 • 9 years ago

Comment on attachment 8604268 [details]
MozReview Request: bz://1153407/jaoo

/r/8583 - Bug 1161072 - Reset docshell state (disabled js/cache, service workers) from actor instead of client. r=jryans
/r/8585 - Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau

Pull down these commits:

hg pull -r 08cc0c4e5b5160bd3793b230abcd7e2299633607 https://reviewboard-hg.mozilla.org/gecko/

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 5 • 9 years ago

Comment on attachment 8604268 [details]
MozReview Request: bz://1153407/jaoo

/r/8583 - Bug 1161072 - Reset docshell state (disabled js/cache, service workers) from actor instead of client. r=jryans
/r/8585 - Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau

Pull down these commits:

hg pull -r 08cc0c4e5b5160bd3793b230abcd7e2299633607 https://reviewboard-hg.mozilla.org/gecko/

	Alexandre Poirot [:ochameau]
	Comment 6 • 9 years ago

https://reviewboard.mozilla.org/r/8585/#review7231

I'm not sure you need a dedicated frame script for this, see my comments.

::: browser/devtools/framework/test/head.js:280
(Diff revision 2)
> +  return executeInContent("devtools:test:waitForSWMessage", {selector});

Note that waitForSWMessage looks like Test:SynthesizeMouse from frame-script-utils.js.
Both meant to click on a node.
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/frame-script-utils.js#195

::: browser/devtools/framework/test/head.js:284
(Diff revision 2)
> +  return executeInContent("devtools:test:unregisterSW", {selector}, {}, false);

It also looks like SynthesizeMouse, but here you may also just call unregister() method directly by using eval devtools:test:eval:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/frame-script-utils.js#106

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 7 • 9 years ago

https://reviewboard.mozilla.org/r/8585/#review7233

> Note that waitForSWMessage looks like Test:SynthesizeMouse from frame-script-utils.js.
> Both meant to click on a node.
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/frame-script-utils.js#195

That won't work. Note `waitForSWMessage` requests content to wait for the click event whereas `SynthesizeMouse` requests content to click (if I understand this correctly).

> It also looks like SynthesizeMouse, but here you may also just call unregister() method directly by using eval devtools:test:eval:
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/frame-script-utils.js#106

Nice, a `devtools:test:eval` message will do the work then.

	Jordan Santell [:jsantell] [@jsantell] Reporter
	Comment 8 • 9 years ago

I think Alex answered your question, but let me know if you have any other questions! Pretty much using the browser/devtools/shared/frame-script-utils.js in the content proc should do it.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 9 • 9 years ago

The bits we needed here from bug 1161072 have already landed. It's time to go for this. Alex, would you mind to review this now please? Thanks!

PS The mozreview thing is broken on my side. I'll try to recovery it ASAP.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 10 • 9 years ago

Comment on attachment 8604268 [details]
MozReview Request: bz://1153407/jaoo

/r/8583 - Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau

Pull down this commit:

hg pull -r 85472ac5063a0e52901322a5b6fe49dc97c12a0a https://reviewboard-hg.mozilla.org/gecko/

	Alexandre Poirot [:ochameau]
	Comment 11 • 9 years ago

Comment on attachment 8604268 [details]
MozReview Request: bz://1153407/jaoo

https://reviewboard.mozilla.org/r/8581/#review8065

Supporting e10s introduces some additional complexity, but I think the test itself is unnecessarily complex, because it uses DOM to set a state and clicks as a way to send messages.
The frame script can just call the navigator.serviceWorker and set its own listeners instead of hacking around DOM and click.
It will make the test be non-visual, but much more easy to understand. (First reaction when reading it: Why are we listening for clicks for a service worker tests?)
My second reaction was like your frame script was unecessary and not service worker specific (first review). It is still the case as it does some very generic stuff like clicks. It would make it much more clearer if it was calling serviceWorker API!

::: browser/devtools/framework/test/browser_toolbox_options_enable_serviceworkers_testing_frame_script.js:23
(Diff revision 3)
> +  }

I'm not sure you should reply if there is no node, it sounds more like a error. And here we are going to hide an error by doing that.

::: browser/devtools/framework/test/browser_toolbox_options_enable_serviceworkers_testing_frame_script.js:37
(Diff revision 3)
> +});

Looks like a leftover

::: browser/devtools/framework/test/browser_toolbox_options_enable_serviceworkers_testing_frame_script.js:63
(Diff revision 3)
> +}

All framescripts might be evaluated in the same scope, so superQuerySelector may already be defined by the common frame script...

::: browser/devtools/framework/test/browser_toolbox_options_enable_serviceworkers_testing.js:75
(Diff revision 3)
> -  }
> +    }

All these `if (output.textContenxt !== "No output")` look wrong. Shouldn't we assert only one particular behavior when running the test?
These tests seriously complexify the readability of this test.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 12 • 9 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #11)
> Comment on attachment 8604268 [details]
> MozReview Request: bz://1153407/jaoo
> 
> https://reviewboard.mozilla.org/r/8581/#review8065
> 
> Supporting e10s introduces some additional complexity, but I think the test
> itself is unnecessarily complex, because it uses DOM to set a state and
> clicks as a way to send messages.
> The frame script can just call the navigator.serviceWorker and set its own
> listeners instead of hacking around DOM and click.

The reason for it is because we don't support chrome service workers yet so this is the only way to test this at the moment.

	Alexandre Poirot [:ochameau]
	Comment 13 • 9 years ago

The frame script has access to content and should be able to call navigator.serviceWorker like this:
 
  content.navigator.serviceWorker.xxx()

	Alexandre Poirot [:ochameau]
	Comment 14 • 9 years ago

The frame script executes with chrome privileges, but can call content service worker.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 15 • 9 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #13)
> The frame script has access to content and should be able to call
> navigator.serviceWorker like this:
>  
>   content.navigator.serviceWorker.xxx()

It seems it doesn't work. Could you have a look at it and let me know whether that's correct please? It hits the following (chrome service worker are not allowed yet).

Assertion failure: !nsContentUtils::IsCallerChrome(), at /Volumes/firefoxos/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp:1054

	Alexandre Poirot [:ochameau]
	Comment 16 • 9 years ago

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #15)
> Created attachment 8610689 [details] [diff] [review]
> content.navigator.serviceWorker.xxx() way patch
> 
> (In reply to Alexandre Poirot [:ochameau] from comment #13)
> > The frame script has access to content and should be able to call
> > navigator.serviceWorker like this:
> >  
> >   content.navigator.serviceWorker.xxx()
> 
> It seems it doesn't work. Could you have a look at it and let me know
> whether that's correct please? It hits the following (chrome service worker
> are not allowed yet).
> 
> Assertion failure: !nsContentUtils::IsCallerChrome(), at
> /Volumes/firefoxos/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp:
> 1054

... this assertion is wrong, I opened bug 1153407 to ease this assertion and prevent throwing on such regular usage! There is nothing wrong in calling service worker of a *content* document from *chrome* context.

You could workaround that by using eval, but that sounds sad.
  content.eval("navigator.serviceWorker.xxx()")
It may end up being as complex as your previous patch with clicks...
  let promise = content.eval("navigator.serviceWorker.register()");
  promise.then(..., ...);

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 17 • 9 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #16)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #15)
> > Created attachment 8610689 [details] [diff] [review]
> > content.navigator.serviceWorker.xxx() way patch
> > 
> > (In reply to Alexandre Poirot [:ochameau] from comment #13)
> > > The frame script has access to content and should be able to call
> > > navigator.serviceWorker like this:
> > >  
> > >   content.navigator.serviceWorker.xxx()
> > 
> > It seems it doesn't work. Could you have a look at it and let me know
> > whether that's correct please? It hits the following (chrome service worker
> > are not allowed yet).
> > 
> > Assertion failure: !nsContentUtils::IsCallerChrome(), at
> > /Volumes/firefoxos/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp:
> > 1054
> 
> ... this assertion is wrong, I opened bug 1153407 to ease this assertion and
> prevent throwing on such regular usage! There is nothing wrong in calling
> service worker of a *content* document from *chrome* context.

Thanks for your help here. Once bug 1169210 gets landed I'll rewrite this test so we could remove the complexity of it.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 18 • 9 years ago

Bug 1169210 - Can't call content service worker from chrome. r=nsm

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 19 • 9 years ago

Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 20 • 9 years ago

Comment on attachment 8613468 [details]
MozReview Request: Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau

Bug 1169210 - Can't call content service worker from chrome. r=nsm

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 21 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6783f502863f

	Alexandre Poirot [:ochameau]
	Comment 22 • 9 years ago

https://reviewboard.mozilla.org/r/8581/#review8545

The test is so much simplier, thanks for the c++ fix and keeping addressing my nits!
It just misses the frame script now.

::: browser/devtools/framework/test/browser_toolbox_options_enable_serviceworkers_testing.js:109
(Diff revision 4)
> +    .then(toolbox.destroy)

It will be safer to pass `toolbox.destroy.bind(toolbox)` if destroy uses `this`.

::: browser/devtools/framework/test/browser.ini:15
(Diff revision 4)
> +  browser_toolbox_options_enable_serviceworkers_testing_frame_script.js

This file is missing from this patch

::: browser/devtools/framework/test/browser_toolbox_options_enable_serviceworkers_testing.js:44
(Diff revision 4)
>    content.location = TEST_URI;

If you don't need any special document, you may just load a "data:text/html,SW-test" URL instead.

::: browser/devtools/framework/test/browser_toolbox_options_enable_serviceworkers_testing.js:54
(Diff revision 4)
> -  let deferred = promise.defer();
> +  return executeInContent("devtools:test:register");

It looks like this is defined in your service-worker specific frame script.
It would be great to have a more specific name to prevent name collision with the common frame script. Something like `devtools:sw-test:register`.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 23 • 9 years ago

Comment on attachment 8613468 [details]
MozReview Request: Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau

Bug 1169210 - Can't call content service worker from chrome. r=nsm

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 24 • 9 years ago

Comment on attachment 8613469 [details]
MozReview Request: Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau

Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 25 • 9 years ago

https://reviewboard.mozilla.org/r/8581/#review8567

> This file is missing from this patch

Yeah, my fault. Sorry.

> If you don't need any special document, you may just load a "data:text/html,SW-test" URL instead.

Sadly that URL won't be valid. By using that URL we hit https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerContainer.cpp#101

> It looks like this is defined in your service-worker specific frame script.
> It would be great to have a more specific name to prevent name collision with the common frame script. Something like `devtools:sw-test:register`.

Sure, done.

> It will be safer to pass `toolbox.destroy.bind(toolbox)` if destroy uses `this`.

Done, thanks for the suggestion.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 26 • 9 years ago

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

	Alexandre Poirot [:ochameau]
	Comment 27 • 9 years ago

https://reviewboard.mozilla.org/r/8581/#review8665

::: browser/devtools/framework/test/browser_toolbox_options_enable_serviceworkers_testing_frame_script.js:24
(Diff revision 5)
> +});

\o/ So much better!

	Alexandre Poirot [:ochameau]
	Comment 28 • 9 years ago

Comment on attachment 8613469 [details]
MozReview Request: Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau

https://reviewboard.mozilla.org/r/9741/#review8671

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 29 • 9 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #27)
> https://reviewboard.mozilla.org/r/8581/#review8665
> 
> :::
> browser/devtools/framework/test/
> browser_toolbox_options_enable_serviceworkers_testing_frame_script.js:24
> (Diff revision 5)
> > +});
> 
> \o/ So much better!

Thanks. I'll land this once bug 1169210 gets landed.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 30 • 9 years ago

Comment on attachment 8613468 [details]
MozReview Request: Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau

Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 31 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95ba3de12a6c

Let's see how that looks and land this.

	José Antonio Olivera Ortega [:jaoo] Assignee
	Comment 32 • 9 years ago

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #31)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=95ba3de12a6c
> 
> Let's see how that looks and land this.

Looking good, let's land it.

	Pulsebot
	Comment 33 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/51a031e11595

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/51a031e11595