Closed Bug 1390489 Opened 2 years ago Closed 2 years ago

Port StatementRow to WebIDL

Categories

(Toolkit :: Storage, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
See bug 1389510. Currently StatementRow uses the getProperty class hook we want to remove - if we port to WebIDL it becomes a JS proxy without these hooks.

Andrew are you an appropriate reviewer for this? Please double check the RefPtr usage, I'm not that familiar with it.

This patch also fixes bug 489897, enumeration now works for row objects. I added a test for this.

Green on Try.
Attachment #8897411 - Flags: review?(bugmail)
Andrew, any chance you could look at this soon, or should I try to find another reviewer?

I've fixed all other bugs blocking bug 1389510 so the only places where we use these JS hooks now are in storage/ - StatementRow and (Async)StatementParams :)
Yes, sorry for the delays, I'll have the review done in the next few hours.  We were previously treating this logic as black magic and there's been non-trivial spin-up for me.  (But I think it's worthwhile, because there are potentially major wins if we move more/all of our bindings to WebIDL so that things like Firefox Sync can do all of their DB work on their ChromeWorker without touching the main thread.)
Comment on attachment 8897411 [details] [diff] [review]
Patch

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

r=asuth conditional on squashing my patch or equivalent.  Also note that while I've deeply attempted to understand what's going on here, I'm not a bindings expert yet, and so if you're not feeling confident about the bindings stuff yourself, we probably want a DOM peer to review.

I see you have a params patch on the try stack.  I should be able to turn that review around nearly instantly (to the same level of hand-waving about bindings).

::: storage/mozStorageStatementRow.cpp
@@ -51,2 @@
>  {
> -  NS_ENSURE_TRUE(mStatement, NS_ERROR_NOT_INITIALIZED);

The loss of an equivalent to this introduced a crasher.  I have a fix and a test change in a patch I'll attach.  Please squash or otherwise make sure you land test coverage and the fix.

@@ -126,2 @@
>  
> -  NS_ENSURE_TRUE(mStatement, NS_ERROR_NOT_INITIALIZED);

(same deal)
Attachment #8897411 - Flags: review?(bugmail) → review+
The test correctly fails due to a cras if the fix is not present.
Assignee: jdemooij → bugmail
Not the most useful thing, but helped me realize that mRow was not introducing a change in ownership, just coalescing and replacing some XPConnect wrappers.  (I got thrown off by that and the NS_ENSURE_TRUE removal and thought the changes were more extensive than they were.)
(I need to figure out how to change "hg bzexport"'s defaults to stop stealing bugs...)
Assignee: bugmail → jdemooij
Attached patch Patch v2Splinter Review
Kyle, could you review the bindings part, in particular the dom/ changes? 

See comment 0 for more info on why we're doing this.
Attachment #8897411 - Attachment is obsolete: true
Attachment #8899239 - Flags: review?(kyle)
(In reply to Andrew Sutherland [:asuth] from comment #3)
> The loss of an equivalent to this introduced a crasher.  I have a fix and a
> test change in a patch I'll attach.  Please squash or otherwise make sure
> you land test coverage and the fix.

Great catch! I think I got this right in the params patch; weird I forgot it here because it's the whole reason for the StatementRowHolder part :)

Thanks for the review!
Comment on attachment 8899239 [details] [diff] [review]
Patch v2

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

tiny style nit, otherwise lgtm!

::: storage/mozStorageStatementJSHelper.cpp
@@ +100,3 @@
>    if (!aStatement->mStatementRowHolder) {
> +    dom::GlobalObject global(aCtx, scope);
> +    if (global.Failed())

nit: braces
Attachment #8899239 - Flags: review?(kyle) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89df4227af85
Port StatementRow to WebIDL bindings. r=asuth,qdot
Duplicate of this bug: 489897
https://hg.mozilla.org/mozilla-central/rev/89df4227af85
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Fwiw, there's a slight behavior change here for the getter: we used to byte-deflate, now we convert UTF16 to UTF8.  Are non-ASCII column names used in practice?
Flags: needinfo?(bugmail)
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #13)
> Fwiw, there's a slight behavior change here for the getter: we used to
> byte-deflate, now we convert UTF16 to UTF8.  Are non-ASCII column names used
> in practice?

In practice, non-ASCII column names are not used, no.  If you find any (outside of legacy extensions that took it upon themselves to expose WebSQLDatabase or similar), it suggests the column names are content-controlled and we've got a security problem.  However, mozStorage and SQLite explicitly support utf-8 columns names, so enhancing our scope to support non-ASCII/non-Latin1 things is a win.

Restating: I believe you're referring to the change from using JSAutoByteString which uses JS_EncodeString under the hood which uses EncodeLatin1 under that hood.  And that branches based on some optimized string magic, but if we've got 2-byte chars, uses LossyTwoByteCharsToNewLatin1CharsZ which explicitly truncates (through a static_cast<unsigned char>).  And mozStorage is cool with doing less truncation.

Thanks very much for checking, it's definitely good to have wizard-level confirmation that this all does what we want!  If I never have to test patches for release branch uplift again, I'll be a happy human.
Flags: needinfo?(bugmail)
Blocks: 958644
You need to log in before you can comment on or make changes to this bug.