1205205 - Crash when asynchronously executing a mozIStorageStatement containing a user defined function call

Status: NEW

(Toolkit :: Storage, defect, P3)

Description

[slemmer.balazs]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150826185918

Steps to reproduce:

1. Create mozIStorageConnection
2. Create udf using createFunction()
3. Prepare a statement containing a call to the previously created udf
4. Execute the statement asynchronously


Actual results:

The application crashes when mozIStorageStatement.executeAsync() or mozIStorageConnection.executeAsync() is called.


Expected results:

The statement should be executed without crashing, returning the results of the query containing the udf call.
This works when using the synchronous mozIStorageStatement.executeStep().

Comment 1

[Loic]

Could you attach a minimal testcase, please.

Comment 2

[slemmer.balazs]

Attachment: Barebones example legacy extension

Attached a barebones example legacy extension. Test is at chrome://udf/content/udf.xul

Comment 3

[Loic]

CR: https://crash-stats.mozilla.com/report/index/9e7f96ac-ab59-42a4-b939-0d8b32150916

It didn't crash before:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=86413e921d5d&tochange=0414d6d0f60d
Suspected bug:
Bobby Holley — Bug 770840 - Add Runtime aborts when using XPCWrappedJS off-main-thread. v2

Comment 4

[Bobby Holley (:bholley)]

Bug 770840 just means we crash when using an XPCWrappedJS off the main thread (which is unsafe).

I'm assuming that the attached extension provides a JS implementation of an interface that the platform assumes is C++-implemented. Presumably we should mark any such interfaces as [builtinclass] so that we can fail more gracefully here.

Who owns storage now? Jan, do you know anything about this?

Comment 5

[Jan Varga [:janv]]

(In reply to Bobby Holley (:bholley) from comment #4)
> Bug 770840 just means we crash when using an XPCWrappedJS off the main
> thread (which is unsafe).
> 
> I'm assuming that the attached extension provides a JS implementation of an
> interface that the platform assumes is C++-implemented. Presumably we should
> mark any such interfaces as [builtinclass] so that we can fail more
> gracefully here.
> 
> Who owns storage now? Jan, do you know anything about this?

I don't own storage, but what you said makes sense to me.
Marco has been doing reviews of storage stuff related to IndexedDB.

Comment 6

[Marco Bonardo [:mak]]

Yes, I de-facto own Storage. I assume the add-on is creating a function in js, registering it, and then Storage tries to use it off the main-thread, because of the async APIs.

These are likely going through createFunction and createAggregateFunction and the interfaces are mozIStorageFunction or mozIStorageAggregateFunction. Originally this path was fine cause all of the APIs were synchronous, so everything was running on the main thread.

Though, as it is now (the synchronous API should _IDEALLY_ be used off the main thread, so not an interesting case), we basically don't support js implemented SQL functions. And I don't think this will change, cause the alternative would be to block the querying thread just to run some code on the main-thread, that sounds like janky.

If [builtinclass] makes so that we don't accept js implementations of the given interface, I'm all for it.

I don't know much about this decorator, and mdn doesn't help.
Can you please point me to some documentation about it, or just briefly explain its effect and how it's going to improve the situation? Thanks.

Comment 7

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

(In reply to Marco Bonardo [::mak] from comment #6)
> Yes, I de-facto own Storage. I assume the add-on is creating a function in
> js, registering it, and then Storage tries to use it off the main-thread,
> because of the async APIs.

You're more than de facto, you're the fully official owner!

fwiw, we talked about this some in bug 702559 comment 19, although at the time I guess I only knew about [noscript].  [builtinclass] is what we want to prevent script from implementing the interface, see the table at https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL#Interfaces.  (And I think we still want [scriptable] so JS code can at least move native functions around.  One might even argue we should put [noscript] on the methods because they're really not intended to be called by anything but SQLite too, but that might be overdoing it.)

I agree that it makes sense to entirely mark the interfaces as [builtinclass].  It is currently safe to have custom JS functions on the main thread, so we will be removing functionality, but we also absolutely don't want SQLite running on the main thread at all.  I could see people intentionally doing main-thread things to get the custom function functionality.  (In fact, when searching my mail, I found one related discussion on this in 2010 where someone absolutely did do this.)

When doing this, it might also be appropriate to consider turning on http://sqlite.org/json1.html as a migration path, since it's conceivable this is an existing use-case.  However, my vote would be to say "no" to that until we have an in-tree need.  In general, I believe we want to be directing would-be mozStorage users to use indexedDB which has compressed structured clone magic and whose indices with named paths are capable of meeting most use-cases in a way that avoids foot-guns.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Marco Bonardo [::mak] from comment #6)
> If [builtinclass] makes so that we don't accept js implementations of the
> given interface, I'm all for it.

That is exactly what it does.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'nsXPCWrappedJS::CallMethod':
 - nightly (version 50): 0 crash from 2016-06-06.
 - aurora  (version 49): 1 crash from 2016-06-07.
 - beta    (version 48): 18 crashes from 2016-06-06.
 - release (version 47): 32 crashes from 2016-05-31.
 - esr     (version 45): 1 crash from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          0          0          0          0          0
 - aurora           0          1          0          0          0          0          0
 - beta             2          4          5          0          2          3          1
 - release          9          3          8          2          2          3          2
 - esr              1          0          0          0          0          0          0

Affected platform: Windows

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Since the crash volume is low (less than 15 per week), the severity is downgraded to S3. Feel free to change it back if you think the bug is still critical.

For more information, please visit auto_nag documentation.