Closed
Bug 1154325
Opened 9 years ago
Closed 9 years ago
wpt cache-storage-keys tests fail with "unpaired\uD800" cache key
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
Attachments
(3 files, 3 obsolete files)
6.44 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
17.89 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
29.93 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
I suspect the default encoding in sqlite is a problem here. I will try explicitly setting to UTF16.
Assignee | ||
Comment 2•9 years ago
|
||
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
Assignee | ||
Comment 3•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/525442ca000d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 6•9 years ago
|
||
This bug is to re-enable and fix the problem. So re-opening.
Status: RESOLVED → REOPENED
status-firefox40:
fixed → ---
Flags: needinfo?(bkelly)
Resolution: FIXED → ---
Updated•9 years ago
|
Keywords: leave-open
Comment 10•9 years ago
|
||
I disabled all WPT DOM Cache tests for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/86a2a7ecc011
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1dd2bcef86d6 https://hg.mozilla.org/mozilla-central/rev/9eda61f45773 https://hg.mozilla.org/mozilla-central/rev/86a2a7ecc011
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8596829 -
Flags: review?(bugmail)
Assignee | ||
Comment 13•9 years ago
|
||
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.
Assignee | ||
Updated•9 years ago
|
Attachment #8596831 -
Flags: review?(ehsan)
Comment 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
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 17•9 years ago
|
||
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+
Comment 18•9 years ago
|
||
(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.)
Assignee | ||
Comment 19•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8597460 -
Flags: review?(james) → review+
Assignee | ||
Comment 20•9 years ago
|
||
I'd like to land bug 1120501 before landing this one.
Depends on: 1120501
Assignee | ||
Comment 21•9 years ago
|
||
Attachment #8597262 -
Attachment is obsolete: true
Attachment #8598155 -
Flags: review+
Assignee | ||
Comment 22•9 years ago
|
||
Removed a FAIL from the sandboxed test now that bug 1158319 has landed.
Attachment #8597460 -
Attachment is obsolete: true
Attachment #8598156 -
Flags: review+
Comment 23•9 years ago
|
||
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
https://hg.mozilla.org/mozilla-central/rev/0ee4a1086547 https://hg.mozilla.org/mozilla-central/rev/4583badf15cf https://hg.mozilla.org/mozilla-central/rev/711f6e43d446
Assignee | ||
Updated•9 years ago
|
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Blocks: 1160389
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•