Closed Bug 1154325 Opened 5 years ago Closed 5 years ago

wpt cache-storage-keys tests fail with "unpaired\uD800" cache key

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(3 files, 3 obsolete files)

Investigate why the cache-storage.https.html tests fail the "CacheStorage names are DOMString not USVStrings" test.  Also note that these values breakcache-storage-keys.https.html when it attempts to cleanup the "unpaired\uD8000" value.
I suspect the default encoding in sqlite is a problem here.  I will try explicitly setting to UTF16.
UTF-16 encoding sqlite fixes this.  Kind of sucks to pessimize database size for such a corner case, though...
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Actually, I can avoid that by using Blob for the key type in the database.  Just makes the code a bit clunky.
I disabled cache-storage-keys.https.html and bkelly said to use this bug to track re-enabling it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/525442ca000d
Flags: needinfo?(bkelly)
https://hg.mozilla.org/mozilla-central/rev/525442ca000d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
This bug is to re-enable and fix the problem.  So re-opening.
Status: RESOLVED → REOPENED
Flags: needinfo?(bkelly)
Resolution: FIXED → ---
Keywords: leave-open
I disabled all WPT DOM Cache tests for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/86a2a7ecc011
I will investigate the performance impact of using blobs for all strings in bug 1110458.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78a2472c527d

I also still need a follow-on patch here to re-enable the relevant WPT test or add a mochitest that triggers the same problem.
Attachment #8596831 - Flags: review?(ehsan)
Comment on attachment 8596831 [details] [diff] [review]
P2 Use Blobs for CacheStorage keys to avoid encoding issues. r=ehsan

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

::: dom/cache/DBSchema.cpp
@@ +1193,5 @@
>  
>    rv = BindId(state, 16, aResponseBodyId);
>    if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>  
> +  rv = state->BindUTF8StringAsBlobParameter(17, aResponse.securityInfo());

Nice!

@@ +1328,5 @@
>      rv = ExtractId(state, 5, &aSavedResponseOut->mBodyId);
>      if (NS_WARN_IF(NS_FAILED(rv))) { return rv; }
>    }
>  
> +  rv = state->GetBlobAsUTF8String(6, aSavedResponseOut->mValue.securityInfo());

Very nice!
Attachment #8596831 - Flags: review?(ehsan) → review+
Comment on attachment 8596829 [details] [diff] [review]
P1 Add convenience routines to bind and get Blobs as strings. r=asuth

Try shows the new methods leak.  Let me investigate.
Attachment #8596829 - Flags: review?(bugmail)
The string should Adopt() the buffer as GetBlob() passes ownership.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06161dc9b984
Attachment #8596829 - Attachment is obsolete: true
Attachment #8597262 - Flags: review?(bugmail)
Comment on attachment 8597262 [details] [diff] [review]
P1 Add convenience routines to bind and get Blobs as strings. r=asuth

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

Templates for the win!  (It's nice how your use of templates eliminates boilerplate-y code.)

Since the service worker cache logic clearly already has test failures for this and is implementing full coverage/usage of the new logic, I'm signing off on it being okay for there to not be new storage coverage for this.  Especially since the motivating use-case for this is complicated string encoding stuff that's way above mozStorage's pay grade and our existing tests for bindBlobByName are in JS which adds even more nuances I don't think we want/need to get into.

::: storage/public/mozIStorageStatement.idl
@@ +215,5 @@
> +   * @param aIndex
> +   *        0-based colummn index.
> +   * @return The value for the result Blob column interpreted as a String.
> +   *         No encoding conversion is performed.
> +   * @{

The @{ and @} aren't necessary here and in the next comment, please remove.  The existing usage was to try and reuse a comment across two getters.
Attachment #8597262 - Flags: review?(bugmail) → review+
(cc'ing mak for awareness of attachment 8597262 [details] [diff] [review] adding string-as-blob convenience helpers to mozStorage for speak-now-or-complain-less-later purposes since this bug is outside the storage component.  :bent is already aware of the use-case from IRC and probably watching this component.)
This patch re-enables the cache-storage wpt tests.  I added the following bugs for issues found:

  bug 1158262
  bug 1158265
  bug 1158263
  bug 1158292
  bug 1158301
  bug 1158306
  bug 1158314
  bug 1158324
  bug 1158319
  bug 1158347

Note, I left the serviceworker tests disabled as the MessageChannel reference error prevents any of them from running anyway.  The previous ini settings were a bit deceptive in that it made it look like only one test failed, but in fact none of them ran.

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=88e544758197
Attachment #8597460 - Flags: review?(james)
Attachment #8597460 - Flags: review?(james) → review+
I'd like to land bug 1120501 before landing this one.
Depends on: 1120501
Removed a FAIL from the sandboxed test now that bug 1158319 has landed.
Attachment #8597460 - Attachment is obsolete: true
Attachment #8598156 - Flags: review+
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Depends on: 1248757
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.