1406112 - Statement params getter doesn't return previously set value

Status: NEW

(Toolkit :: Storage, defect, P5)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Testcase - v1

bug 1392554 made changes to the statement wrappers, which unfortunately changes the behavior of the params getters/setters. Setting a statement parameter and then using its getter doesn't yield the same result. See attached testcase.

As this is causing a pretty severe issue in Lightning I'd appreciate a quick fix, but in the worst case I can work around it by not using the getter.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I just read the comment in bug 1392554's patch "Unfortunately there's no API that lets us return the parameter value". In that case I think it would be better to throw instead of silently fail, as this is unclear from simple API usage.

Comment 2

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

(In reply to Philipp Kewisch [:Fallen] from comment #1)
> I just read the comment in bug 1392554's patch "Unfortunately there's no API
> that lets us return the parameter value". In that case I think it would be
> better to throw instead of silently fail, as this is unclear from simple API
> usage.

From  bug 1392554 comment 0: "Unfortunately we can't throw an exception because setting these properties will also call the getter first."

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Hmm I see, thanks for the hint. I suspect there is no straightforward way to not call the getters first?

Comment 4

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

My suggestion here would be, sequentially:
- Quickly hack something up to not look at the values just written to the magic WebIDL binding.
- Over time, try and move to SQLite.jsm.  Since it takes straight-up normal JS objects/arrays, reading out of them is safe.

I think our general in-tree mozStorage goal is for all JS consumers to be using SQLite.jsm.  It helps hide foot-guns, and will give us a lot more latitude if/when we move SQLite.jsm to operating exclusively using WebIDL bindings.  If/when that day comes, I expect we would try and leverage SQLite.jsm's simpler API surface and the power of WebIDL to drastically simplify what's exposed from C++ to JS.  In that case, if you're not on SQLite.jsm and Thunderbird hasn't forked Gecko yet, there could be worse breakage.

The other option, of course, is to enhance the bindings to support reading the values out, but I'm a bit conflicted on that given that Thunderbird/Lightning would need to do the engineering work and I think that effort is better spent migrating to something more supported and more shim-able.  (In the event we do something crazy in mozilla-central, you could fork SQLite.jsm with a lot less effort than you can fork the C++/XPCOM mozStorage stuff.)

Comment 5

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

(In reply to Philipp Kewisch [:Fallen] from comment #3)
> Hmm I see, thanks for the hint. I suspect there is no straightforward way to
> not call the getters first?

The JS engine is doing that and bug 1392554 was entirely about them removing specialized magic.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Andrew Sutherland [:asuth] from comment #4)
> The other option, of course, is to enhance the bindings to support reading
> the values out, but I'm a bit conflicted on that given that
> Thunderbird/Lightning would need to do the engineering work 
While it is true that this bug was filed due to a Lightning issue, just the fact that it was (again) found there shouldn't be the driver to have that team work on this issue. I respect your storage goals and agree using SQLite.jsm is more preferable, but I think that not providing a getter fundamentally breaks the expectations developers would have to javascript objects. I think this is a bug that should be fixed regardless, but I'll leave it to your discretion if you want to WONTFIX or how to prioritize.

For Lightning I'm going to work around this issue, the patch for that is fairly small. Thank you for the input and quick responses!