caches.match({name: 'badName' }) should resolve undefined, not reject

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bkelly, Assigned: tt)

Tracking

(Blocks 1 bug)

48 Branch
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 1 obsolete attachment)

We're changing the spec not to reject caches.match() if the named cache is not found.  See:

https://github.com/slightlyoff/ServiceWorker/issues/891

This just involves changing a line or two in dom/cache/DBCache.cpp and updating a few tests.
(Assignee)

Comment 1

3 years ago
I'd like to take this bug.
Assignee: nobody → ttung
(Assignee)

Comment 2

3 years ago
In my naive guess, just change the return value [1] to NS_OK and tests which is related to it(test_cache_match*)?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/cache/DBSchema.cpp#904
Essentially yes.  There are probably some wpt tests that exercise this to.

I think we should also add to these tests a caches.has() call after the caches.match() to demonstrate that the Cache object is not created as a side effect.

Does that make sense?
Status: NEW → ASSIGNED
(Assignee)

Comment 4

3 years ago
Yes, thank you for your feedback! I'll resolve it as soon as possible.
Whiteboard: btpp-active
(Assignee)

Comment 5

3 years ago
Do you mind helping me to review this patch, Ben? Thanks! 

In this patch, I changed a line of code to make caches.match resolves to undefined as CacheName is wrong. 

I just modify the return value to NS_OK when the cache name is not be found. Besides, three existing tests which are related to this modification are changed. I modify those tests from expected for rejected to resolved to undefined as caches.match with unopened CacheName. I also add tests to check whether the cache is create after caches.match resolves to undefined as suggestion in comment 3. 

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8e7aabc8caa
Attachment #8751178 - Flags: review?(bkelly)
Comment on attachment 8751178 [details] [diff] [review]
Bug 1271069 - caches.match with wrong name should resolce to undefined.

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

Looks good!  r=me with the one comment addressed.

Thanks!

::: testing/web-platform/tests/service-workers/cache-storage/script-tests/cache-storage-match.js
@@ +111,5 @@
>            return self.caches.match(transaction.request, {cacheName: 'foo'});
>          })
>        .then(function(response) {
> +          assert_equals(response, undefined,
> +                        'The match with bad cache name should reject.');

Please fix this assert message to say "should resolve to undefined".
Attachment #8751178 - Flags: review?(bkelly) → review+
(Assignee)

Comment 7

3 years ago
Thanks for your review and your time, Ben! I addressed the comment.
Attachment #8751178 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5d493625548f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.