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

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I suspect the default encoding in sqlite is a problem here.  I will try explicitly setting to UTF16.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

UTF-16 encoding sqlite fixes this.  Kind of sucks to pessimize database size for such a corner case, though...

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Actually, I can avoid that by using Blob for the key type in the database.  Just makes the code a bit clunky.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/525442ca000d

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

This bug is to re-enable and fix the problem.  So re-opening.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd2bcef86d6

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9eda61f45773

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86a2a7ecc011

Comment 10

[(no longer active)]

I disabled all WPT DOM Cache tests for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/86a2a7ecc011

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1dd2bcef86d6
https://hg.mozilla.org/mozilla-central/rev/9eda61f45773
https://hg.mozilla.org/mozilla-central/rev/86a2a7ecc011

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add convenience routines to bind and get Blobs as strings. r=asuth

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Use Blobs for CacheStorage keys to avoid encoding issues. r=ehsan

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.

Comment 14

[(no longer active)]

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!

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add convenience routines to bind and get Blobs as strings. r=asuth

The string should Adopt() the buffer as GetBlob() passes ownership.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06161dc9b984

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

(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.)

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Re-enable service-workers/cache-storage WPT tests. r=jgraham

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

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

I'd like to land bug 1120501 before landing this one.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add convenience routines to bind and get Blobs as strings. r=asuth

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Re-enable service-workers/cache-storage WPT tests. r=jgraham

Removed a FAIL from the sandboxed test now that bug 1158319 has landed.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee4a1086547
https://hg.mozilla.org/integration/mozilla-inbound/rev/4583badf15cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/711f6e43d446

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ee4a1086547
https://hg.mozilla.org/mozilla-central/rev/4583badf15cf
https://hg.mozilla.org/mozilla-central/rev/711f6e43d446