Phil Ringnalda (:philor) Reporter
	Description • 8 years ago

https://treeherder.mozilla.org/logviewer.html#?job_id=27797414&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=27799458&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=27799473&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=27801486&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=27816639&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=27819380&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=27819380&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=27857601&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=27868440&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=27871124&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=27884945&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=27888264&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=27889070&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=9291190&repo=fx-team
https://treeherder.mozilla.org/logviewer.html#?job_id=9303348&repo=fx-team

	Tom Tung [:tt, :ttung] Assignee
	Comment 1 • 8 years ago

Sorry for the bug. :(
Take this bug since I create the test. But, It looks strange, I'll try to reproduce it first.

	Tom Tung [:tt, :ttung] Assignee
	Comment 3 • 8 years ago

It seems that "cache function", which we use in fetch-rewrite-worker.js, have a problem. The problem happens when we are using "Date.now()" and get the same value among different promises (called from fetch-request-redirect.https.js). I expected they were different between different tests, but it wasn't. Thus, using the specific different number for fetch-rewrite-worker.js rather than using Date.now() may solve this problem. 

Besides, I found that I wrote the test with bad indent, so I correct the indent for these two file. Sorry for that. :(

I'll send this patch for review once passing tryserver.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3df66c7762cc&selectedJob=21041098

	Tom Tung [:tt, :ttung] Assignee
	Comment 4 • 8 years ago

Hi Ben, 

Attachment is my patch for this bug. I try to reproduce this bug by using rr chaos mode. It seems like Date.now() at fetch-rewirte-worker.js will create same value for different tests. Hence, it cause the bug happen. 

Actually, I only add a value for "&cache" to avoid using Date.now(). The other change is re-indent the code since bad indent for last patch from bug 1243792.

Right now, I cannot reproduce bug after applying this patch via rr chaos mode and the try-result seems that it's ok for this bug. Thus, I think the bug is fixed by this patch. Could you help me to review this patch? Thanks!

	Ben Kelly [:bkelly, not reviewing]
	Comment 5 • 8 years ago

Comment on attachment 8754319 [details] [diff] [review]
Bug 1272830 - Intermittent fetch-request-redirect.https.html - test modified.

Sorry, but this patch file is empty.

	Tom Tung [:tt, :ttung] Assignee
	Comment 6 • 8 years ago

Really sorry about sending an empty patch for review. I should check that before sending. :(

	Tom Tung [:tt, :ttung] Assignee
	Comment 7 • 8 years ago

Comment on attachment 8754422 [details] [diff] [review]
Bug 1272830 - Intermittent fetch-request-redirect.https.html - test modified.

This one is real patch. Could you help me to review that, Ben? Thanks!

	Ben Kelly [:bkelly, not reviewing]
	Comment 8 • 8 years ago

Comment on attachment 8754422 [details] [diff] [review]
Bug 1272830 - Intermittent fetch-request-redirect.https.html - test modified.

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

This works, but I wonder if we should should try to fix the cache uniqueness within the worker.  For example, you're not updating other test cases to use the new explicit cache name.  They might still break, etc.

::: testing/web-platform/tests/service-workers/service-worker/fetch-request-redirect.https.html
@@ +175,5 @@
>          });
>    }, 'Verify redirect mode of Fetch API and ServiceWorker FetchEvent.');
>  
> +// test for reponse.redirected
> +promise_test(function(t) {

In the future it would help to put reformatting changes like changing indent in a separate patch.  Its hard to tell what is a functional change with a lot of style changes mixed in.

@@ +302,5 @@
>                    frame.contentWindow.xhr(
>                        './?url=' + encodeURIComponent(XHR_URL) +
>                        '&expected_redirected=false' +
> +                      '&expected_resolves=true' +
> +                      '&cache'),

Why don't you set cache=1, etc for the first three tests here?

::: testing/web-platform/tests/service-workers/service-worker/resources/fetch-rewrite-worker.js
@@ +129,1 @@
>              var cacheName = "cached-fetches-" + Date.now();

Have you tried just using Performance.now() here?  It has a higher resolution.

Also, you could possible do:

  var cacheName = 'cached-fetches-' + Performance.now() + '-' + evt.request.url;

That would probably provide enough isolation without requiring the main page to send a unique cache name through.

	Tom Tung [:tt, :ttung] Assignee
	Comment 9 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #8)
Thanks for your feedback! It help me a lot!

> This works, but I wonder if we should should try to fix the cache uniqueness
> within the worker.  For example, you're not updating other test cases to use
> the new explicit cache name.  They might still break, etc.

Yeah, you are right! I thought that it might be better to keep original mechanism for other tests, but I was wrong. 


> :::
> testing/web-platform/tests/service-workers/service-worker/fetch-request-
> redirect.https.html
> @@ +175,5 @@
> >          });
> >    }, 'Verify redirect mode of Fetch API and ServiceWorker FetchEvent.');
> >  
> > +// test for reponse.redirected
> > +promise_test(function(t) {
> 
> In the future it would help to put reformatting changes like changing indent
> in a separate patch.  Its hard to tell what is a functional change with a
> lot of style changes mixed in.

Actually, it's my mistake in bug 1243792. I didn't notice that the format is wrong for each promise_test in fetch-request-redirect.https.html. They increase two more unnecessary indents. Should I file another bug for it or just divide it into two patch for this bug?

> @@ +302,5 @@
> >                    frame.contentWindow.xhr(
> >                        './?url=' + encodeURIComponent(XHR_URL) +
> >                        '&expected_redirected=false' +
> > +                      '&expected_resolves=true' +
> > +                      '&cache'),
> 
> Why don't you set cache=1, etc for the first three tests here?
Sorry for that. It should be set for a value. However, I would like to address your comments since it's better. 

> :::
> testing/web-platform/tests/service-workers/service-worker/resources/fetch-
> rewrite-worker.js
> @@ +129,1 @@
> >              var cacheName = "cached-fetches-" + Date.now();
> 
> Have you tried just using Performance.now() here?  It has a higher
> resolution.
> Also, you could possible do:
> 
>   var cacheName = 'cached-fetches-' + Performance.now() + '-' +
> evt.request.url;
> That would probably provide enough isolation without requiring the main page
> to send a unique cache name through.

That's great idea! Thanks! I'll address them in my next patch.

	Tom Tung [:tt, :ttung] Assignee
	Comment 10 • 8 years ago

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

	Tom Tung [:tt, :ttung] Assignee
	Comment 11 • 8 years ago

Addressed comment and create this patch.

	Tom Tung [:tt, :ttung] Assignee
	Comment 12 • 8 years ago

Create this patch for correcting wrong indents and exceeding 80-character line length.

	Tom Tung [:tt, :ttung] Assignee
	Comment 13 • 8 years ago

Comment on attachment 8754680 [details] [diff] [review]
Bug 1272830 - P1 - Intermittent fetch-request-redirect.https.html - test modified.

Hi Ben, do you mind helping me to review this patch? Thanks! I only left the related changes in this patch.

	Ben Kelly [:bkelly, not reviewing]
	Comment 15 • 8 years ago

Comment on attachment 8754680 [details] [diff] [review]
Bug 1272830 - P1 - Intermittent fetch-request-redirect.https.html - test modified.

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

Thanks!  Sorry for the delay in review.  I didn't realize this was one line.

	Tom Tung [:tt, :ttung] Assignee
	Comment 16 • 8 years ago

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

	Tom Tung [:tt, :ttung] Assignee
	Comment 17 • 8 years ago

Thanks for your review and feedback, Ben! 

BTW, patch P2 is for reformat the test, should I put it in this bug or file another bug for it?

	Ben Kelly [:bkelly, not reviewing]
	Comment 18 • 8 years ago

(In reply to Tom Tung [:tt] from comment #17)
> BTW, patch P2 is for reformat the test, should I put it in this bug or file
> another bug for it?

You can just put it in this bug.  Does it need review?

	Tom Tung [:tt, :ttung] Assignee
	Comment 19 • 8 years ago

(In reply to Ben Kelly [:bkelly] from comment #18)
> You can just put it in this bug.  Does it need review?
Yes, I'll send a review request to you. Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 20 • 8 years ago

Comment on attachment 8754741 [details] [diff] [review]
Bug 1272830 - P2 - reformat for fetch-request-redirect.https.html & fetch-rewrite-worker.js

This patch is for reformatting the test. Please help me to review this patch. Thank you, Ben!

	Tom Tung [:tt, :ttung] Assignee
	Comment 21 • 8 years ago

Thanks for your review and your time, Ben!

	Tom Tung [:tt, :ttung] Assignee
	Comment 22 • 8 years ago

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=349b02217efc

	Pulsebot
	Comment 23 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce58234202e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8b3e46732c7

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 8 years ago

For future reference, please try to use commit messages that explain what the patch is actually doing instead of restating the problem being solved. Thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 25 • 8 years ago

Ok, sorry for that. I'll do it next time. Thanks for reminding me that!

	Carsten Book [:Tomcat]
	Comment 26 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ce58234202e9
https://hg.mozilla.org/mozilla-central/rev/a8b3e46732c7