browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: jsantell, Assigned: jaoo)

Tracking

(Blocks 1 bug)

37 Branch
Firefox 41
x86
macOS
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10s+, firefox41 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

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.

Updated

4 years ago
tracking-e10s: --- → +
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.
Blocks: 1134329
Assignee: nobody → jaoo
Depends on: 1161072
Status: NEW → ASSIGNED
/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/
(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!
Flags: needinfo?(jsantell)
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/
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/
Attachment #8604268 - Flags: review?(poirot.alex)
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
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.
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.
Flags: needinfo?(jsantell)
Posted patch v2 (obsolete) — Splinter Review
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.
Attachment #8604268 - Attachment is obsolete: true
Attachment #8604268 - Flags: review?(poirot.alex)
Attachment #8607579 - Flags: review?(poirot.alex)
Attachment #8607579 - Attachment is obsolete: true
Attachment #8607579 - Flags: review?(poirot.alex)
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/
Attachment #8604268 - Attachment is obsolete: false
Attachment #8604268 - Flags: review?(poirot.alex)
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.
Attachment #8604268 - Flags: review?(poirot.alex)
(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.
The frame script has access to content and should be able to call navigator.serviceWorker like this:
 
  content.navigator.serviceWorker.xxx()
The frame script executes with chrome privileges, but can call content service worker.
(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
Attachment #8610689 - Flags: review?(poirot.alex)
Depends on: 1169210
(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(..., ...);
(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.
Attachment #8610689 - Attachment is obsolete: true
Attachment #8610689 - Flags: review?(poirot.alex)
Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau
Attachment #8613469 - Flags: review?(poirot.alex)
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
Attachment #8613468 - Flags: review?(nsm.nikhil)
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`.
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
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
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.
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!
Attachment #8613469 - Flags: review?(poirot.alex) → review+
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
(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.
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
Attachment #8613468 - Attachment description: MozReview Request: Bug 1169210 - Can't call content service worker from chrome. r=nsm → MozReview Request: Bug 1153407 - browser_toolbox_options_enable_serviceworkers_testing.js breaks other tests, needs to be e10s. r=ochameau
Attachment #8613469 - Attachment is obsolete: true
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/51a031e11595
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Attachment #8604268 - Attachment is obsolete: true

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.