Change Sqlite.jsm to bind TypedArrays as Blobs, not common Arrays.

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
Storage
P1
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 1 bug)

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 unaffected, firefox53+ fixed, firefox54+ fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Currently binding an Array tries to bind it as a Blob.
This clashes with bug 483318, that would like to bind Arrays for IN clauses.

We should change Sqlite.jsm to bind only typedArrays as blobs.

We should fix this before FF53 ships, to avoid creating previous usage of Array to bind blobs.
(Assignee)

Updated

5 months ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Tracking 53/54+ so we have alignment befor 53 ships.
tracking-firefox53: ? → +
tracking-firefox54: ? → +

Comment 3

5 months ago
mozreview-review
Comment on attachment 8834072 [details]
Bug 1336944 - Change Sqlite.jsm to bind TypedArrays as Blobs, not common Arrays.

https://reviewboard.mozilla.org/r/110154/#review111496

So there are no non-test consumers right now? Interesting...
Attachment #8834072 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 4

5 months ago
mozreview-review
Comment on attachment 8834072 [details]
Bug 1336944 - Change Sqlite.jsm to bind TypedArrays as Blobs, not common Arrays.

https://reviewboard.mozilla.org/r/110154/#review111500

::: toolkit/modules/tests/xpcshell/test_sqlite.js:1106
(Diff revision 1)
>    const bindings = [
>      {
>        null_col: null,
>        integer_col: 12345,
>        text_col: "qwerty",
> -      blob_col: new Array(256).fill(undefined).map( (value, index) => index % 256 ),
> +      blob_col: new Uint8Array(256).fill(undefined).map( (value, index) => index % 256 ),

Does this work, and/or is filling it with undefined here useful, given the map() call? If it is, shouldn't we fill with 0?
(Assignee)

Comment 5

5 months ago
(In reply to :Gijs from comment #4)
> Does this work, and/or is filling it with undefined here useful, given the
> map() call? If it is, shouldn't we fill with 0?

Yeah, it's the same, afaict:
new Uint8Array(10).fill(undefined)
> Uint8Array [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 ]

I will just change undefined with 0, for clarity.
(Assignee)

Comment 6

5 months ago
oh but yeah, the fill itself is pointless, will just remove it.
Comment hidden (mozreview-request)

Comment 8

5 months ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/cf0424523e69
Change Sqlite.jsm to bind TypedArrays as Blobs, not common Arrays. r=Gijs

Comment 9

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cf0424523e69
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Comment 10

5 months ago
Comment on attachment 8834072 [details]
Bug 1336944 - Change Sqlite.jsm to bind TypedArrays as Blobs, not common Arrays.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1247602 
[User impact if declined]: We'd be shipping an API that is changing the next version
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: just changing Array to Uint8Array, new API
[String changes made/needed]: none
Attachment #8834072 - Flags: approval-mozilla-aurora?
Comment on attachment 8834072 [details]
Bug 1336944 - Change Sqlite.jsm to bind TypedArrays as Blobs, not common Arrays.

Ship new API. Aurora53+.
Attachment #8834072 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 12

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/eae7e45deb55
status-firefox53: affected → fixed
You need to log in before you can comment on or make changes to this bug.