Closed Bug 1272830 Opened 4 years ago Closed 4 years ago

Intermittent fetch-request-redirect.https.html | Verify redirected of Response(Fetch API), Cache API and ServiceWorker FetchEvent. - promise_test: Unhandled rejection with value: object "Error: Redirected XHR should be reject and response shoul..."

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: philor, Assigned: tt)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 5 obsolete files)

Summary: Intermittent etch-request-redirect.https.html | Verify redirected of Response(Fetch API), Cache API and ServiceWorker FetchEvent. - promise_test: Unhandled rejection with value: object "Error: Redirected XHR should be reject and response shoul..." → Intermittent fetch-request-redirect.https.html | Verify redirected of Response(Fetch API), Cache API and ServiceWorker FetchEvent. - promise_test: Unhandled rejection with value: object "Error: Redirected XHR should be reject and response shoul..."
Component: web-platform-tests → DOM: Service Workers
Product: Testing → Core
Blocks: 1243792
Flags: needinfo?(ttung)
Sorry for the bug. :(
Take this bug since I create the test. But, It looks strange, I'll try to reproduce it first.
Assignee: nobody → ttung
Flags: needinfo?(ttung)
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
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!
Attachment #8753882 - Attachment is obsolete: true
Attachment #8754319 - Flags: review?(bkelly)
Comment on attachment 8754319 [details] [diff] [review]
Bug 1272830 - Intermittent fetch-request-redirect.https.html - test modified.

Sorry, but this patch file is empty.
Attachment #8754319 - Flags: review?(bkelly)
Really sorry about sending an empty patch for review. I should check that before sending. :(
Attachment #8754319 - Attachment is obsolete: true
Attachment #8754422 - Flags: review-
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!
Attachment #8754422 - Flags: review- → review?(bkelly)
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.
Attachment #8754422 - Flags: review?(bkelly)
(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.
Addressed comment and create this patch.
Attachment #8754422 - Attachment is obsolete: true
Create this patch for correcting wrong indents and exceeding 80-character line length.
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.
Attachment #8754680 - Flags: review?(bkelly)
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.
Attachment #8754680 - Flags: review?(bkelly) → review+
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?
Attachment #8754680 - Attachment is obsolete: true
(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?
(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!
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!
Attachment #8754741 - Attachment description: Bug 1272830 - P2? - reformat for fetch-request-redirect.https.html & fetch-rewrite-worker.js → Bug 1272830 - P2 - reformat for fetch-request-redirect.https.html & fetch-rewrite-worker.js
Attachment #8754741 - Flags: review?(bkelly)
Attachment #8754741 - Flags: review?(bkelly) → review+
Thanks for your review and your time, Ben!
Attachment #8754741 - Attachment is obsolete: true
Attachment #8755663 - Attachment description: [Final] Bug 1272830 - P1 - Intermittent fetch-request-redirect.https.html - test modified. → [Final] Bug 1272830 - P1 - Intermittent fetch-request-redirect.https.html - test modified. r=bkelly.
Keywords: checkin-needed
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!
Ok, sorry for that. I'll do it next time. Thanks for reminding me that!
https://hg.mozilla.org/mozilla-central/rev/ce58234202e9
https://hg.mozilla.org/mozilla-central/rev/a8b3e46732c7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.