Closed Bug 1377019 Opened 7 years ago Closed 7 years ago

Add a test to ensure that a synthesized document has access to a cookie set before its controlling service worker has been registered

Categories

(Core :: DOM: Service Workers, enhancement)

Product:
Core
Component:
DOM: Service Workers
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(1 file, 1 obsolete file)

Add a test to ensure that a synthesized document has access to a cookie set before its controlling service worker has been registered
5.90 KB, patch
jdm
: review+
Details | Diff | Splinter Review 
      No description provided.
Blocks: 1331680
Assignee: nobody → ehsan
This test is needed for bug 1331680.  The goal is to make sure that the rewrite there sends the cookies at the right time when there is a service worker involved.
NI myself to figure out what this means tomorrow.  Comment 0 does not completely make sense to me.
Flags: needinfo?(bkelly)
Comment on attachment 8882053 [details] [diff] [review]
Add a test to ensure that a synthesized document has access to a cookie set before its controlling service worker has been registered

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

::: dom/workers/test/serviceworkers/fetch/cookie/register.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<script>
> +  function ok(v, msg) {

Unused.

::: dom/workers/test/serviceworkers/test_cookie_fetch.html
@@ +20,5 @@
> +  var iframe;
> +  function runTest() {
> +    iframe = document.querySelector("iframe");
> +    iframe.src = "https://example.com/tests/dom/workers/test/serviceworkers/fetch/cookie/register.html";
> +    var ios;

Unused.

@@ +22,5 @@
> +    iframe = document.querySelector("iframe");
> +    iframe.src = "https://example.com/tests/dom/workers/test/serviceworkers/fetch/cookie/register.html";
> +    var ios;
> +    window.onmessage = function(e) {
> +      if (e.data.status == "ok") {

Unused.

@@ +25,5 @@
> +    window.onmessage = function(e) {
> +      if (e.data.status == "ok") {
> +        ok(e.data.result, e.data.message);
> +      } else if (e.data.status == "registrationdone") {
> +        iframe.src = "https://example.com/tests/dom/workers/test/serviceworkers/fetch/cookie/synth.html";

Could we remove the iframe and use a new one? This should cause any record of the cookies to be removed from the child process and ensure we're testing the right codepaths.
Attachment #8882053 - Flags: review?(josh) → review-
(In reply to Josh Matthews [:jdm] from comment #4)
> ::: dom/workers/test/serviceworkers/fetch/cookie/register.html
> @@ +1,3 @@
> > +<!DOCTYPE html>
> > +<script>
> > +  function ok(v, msg) {
> 
> Unused.

It is used from done().

> @@ +22,5 @@
> > +    iframe = document.querySelector("iframe");
> > +    iframe.src = "https://example.com/tests/dom/workers/test/serviceworkers/fetch/cookie/register.html";
> > +    var ios;
> > +    window.onmessage = function(e) {
> > +      if (e.data.status == "ok") {
> 
> Unused.

It is used from ok().

> @@ +25,5 @@
> > +    window.onmessage = function(e) {
> > +      if (e.data.status == "ok") {
> > +        ok(e.data.result, e.data.message);
> > +      } else if (e.data.status == "registrationdone") {
> > +        iframe.src = "https://example.com/tests/dom/workers/test/serviceworkers/fetch/cookie/synth.html";
> 
> Could we remove the iframe and use a new one? This should cause any record
> of the cookies to be removed from the child process and ensure we're testing
> the right codepaths.

Right, sorry missed that part of your comment.
Attachment #8882053 - Attachment is obsolete: true
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #3)
> NI myself to figure out what this means tomorrow.  Comment 0 does not
> completely make sense to me.

See the review comment in bug 1331680 comment 219.  This test isn't about service workers, but about cookies.  There is a lot of context in that bug but I'm not sure what part of it you are interested.  You can probably talk to Josh and Amy about it at the work week.
Attachment #8882158 - Flags: review?(josh) → review+
Comment on attachment 8882158 [details] [diff] [review]
Add a test to ensure that a synthesized document has access to a cookie set before its controlling service worker has been registered

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

::: dom/workers/test/serviceworkers/fetch/cookie/register.html
@@ +11,5 @@
> +
> +  document.cookie = "foo=bar";
> +
> +  navigator.serviceWorker.ready.then(done);
> +  navigator.serviceWorker.register("cookie_test.js", {scope: "."});

I went through and fixed a lot of tests not to use the ready promise.  It caused a lot of confusion in the past and many tests were racy because of it.  Its preferred now to do something like this instead:

http://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/test_async_waituntil.html#51
Ok.  The way comment 0 says "synthesized document ... before its controlling service worker has been registered" is very confusing.  There cannot be a synthesized document before a service worker has been registered to intercept the FetchEvent.

The test looks reasonable, though, and will be good to make sure we don't break this functionality when we switch to parent-side intercept.
Flags: needinfo?(bkelly)
(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #8)
> Comment on attachment 8882158 [details] [diff] [review]
> Add a test to ensure that a synthesized document has access to a cookie set
> before its controlling service worker has been registered
> 
> Review of attachment 8882158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/test/serviceworkers/fetch/cookie/register.html
> @@ +11,5 @@
> > +
> > +  document.cookie = "foo=bar";
> > +
> > +  navigator.serviceWorker.ready.then(done);
> > +  navigator.serviceWorker.register("cookie_test.js", {scope: "."});
> 
> I went through and fixed a lot of tests not to use the ready promise.  It
> caused a lot of confusion in the past and many tests were racy because of
> it.  Its preferred now to do something like this instead:
> 
> http://searchfox.org/mozilla-central/source/dom/workers/test/serviceworkers/
> test_async_waituntil.html#51

Interesting, I didn't know that.  What's the underlying issue, out of curiosity?  BTW did you know that there are still many test using this pattern? (I used it here because I cargo culted one of them) https://searchfox.org/mozilla-central/search?q=navigator.serviceWorker.ready.then&path=

I'll switch to the new way of doing things here.
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a648e9c98817
Add a test to ensure that a synthesized document has access to a cookie set before its controlling service worker has been registered; r=jdm
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #10)
> Interesting, I didn't know that.  What's the underlying issue, out of
> curiosity?  BTW did you know that there are still many test using this
> pattern? (I used it here because I cargo culted one of them)
> https://searchfox.org/mozilla-central/search?q=navigator.serviceWorker.ready.
> then&path=

Maybe I am mis-remembering.  See bug 1364277.

I guess I prefer the wait-for-state because it works even when the current window is not covered by the scope.  So you don't need all these extra register.html/unregister.html iframes.

> I'll switch to the new way of doing things here.

Thanks.
https://hg.mozilla.org/mozilla-central/rev/a648e9c98817
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: