586237 - Crash when calling the mozIStorageBindingParams methods from a working thread

Status: RESOLVED INVALID

(Core :: XPConnect, defect)

Description

[Adrià Mercader]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b4pre) Gecko/20100810 Minefield/4.0b4pre

When calling a method of mozIStorageBindingParams like bindByName from outside the main thred, the application crashes and exits. If called from the main thread, the code is executed successfully. Also, binding the parameter via mozIStorageStatementParams (stmt.params.xxxx = yyyy) inside a working thread works fine.

Reproducible: Always

Steps to Reproduce:
I've attached a file reproducing the following steps:

1.Create a function that:
a.Opens a storage connection and creates a statement with a binding
b.Creates a new mozIStorageBindingParamsArray object and a mozIStorageBindingParams object and binds the parameter with the bindByName() function.
c. Executes the statement async
2.Run the previous function in a separate thread
Actual Results:  
The application crashes when calling the bindByName() method.

Expected Results:  
Bind the parameter and follow with the statement execution.

The code works on a working thread in Firefox 3.6.8.

The shell console shows the lines included in the file console.txt

Comment 1

[Adrià Mercader]

Attachment: Test case to reproduce the crash (run test_thread_DB())

Comment 2

[Adrià Mercader]

Attachment: Database to use with the test

Comment 3

[Adrià Mercader]

Attachment: Console output after crash

Comment 4

[Benjamin Smedberg]

Have you got a stack trace or crash report ID?

Comment 5

[Adrià Mercader]

Attachment: Stack trace

Comment 6

[Adrià Mercader]

(In reply to comment #4)
> Have you got a stack trace or crash report ID?

I've attached a stack trace created following this instructions:

http://kb.mozillazine.org/Getting_a_stacktrace_with_gdb

Please let me know if this is not what you asked me.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> When calling a method of mozIStorageBindingParams like bindByName from outside
> the main thred, the application crashes and exits. If called from the main
> thread, the code is executed successfully. Also, binding the parameter via
> mozIStorageStatementParams (stmt.params.xxxx = yyyy) inside a working thread
> works fine.
You should not be using the stmt.params.xxxx methods on a different thread because it uses XPConnect, which AFAIK is main thread only.

> 1.Create a function that:
> a.Opens a storage connection and creates a statement with a binding
> b.Creates a new mozIStorageBindingParamsArray object and a
> mozIStorageBindingParams object and binds the parameter with the bindByName()
> function.
> c. Executes the statement async
> 2.Run the previous function in a separate thread
> Actual Results:  
None of this should be illegal per the code.

So, a few things concern me:
1) is this code being run from a window, or a JS component?
2) The error log you attach says there are aborts from the cycle collector being used off of the main thread.  Storage is not hooked up to the cycle collector, so it isn't the cause of those.

Comment 8

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

XPCVariants are cycle collected, and the exploding bind call uses them...

Comment 9

[Shawn Wilsher :sdwilsh]

hurg.  We may need to change that to the new jsval for idls then :/

Still need my question to (1) answered though.

Comment 10

[timeless]

xpconnect was *not* supposed to be main thread only. it historically did support threaded stuff. your code should recognize it's not on the main thread and either throw an exception or return some other object.

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> xpconnect was *not* supposed to be main thread only. it historically did
> support threaded stuff. your code should recognize it's not on the main thread
> and either throw an exception or return some other object.
Alright, maybe my assumption was wrong.  Still need the answer to (1) since if it's running from a window, that isn't threadsafe (and is cycle collected).

Comment 12

[timeless]

sure.

Comment 13

[timeless]

reporter: sorry, the stack trace you provided is useless.

https://developer.mozilla.org/en/How_to_get_a_stacktrace_for_a_bug_report#Linux

Please note that if you are using a distro build you need to install their symbols. If you're using a 32bit build we provide, crash-reporter should work. if you're using your own build, you need --disable-strip and --enable-debugger-info-modules/--enable-debug (preferably the latter since it'll complain sooner).

Comment 14

[Adrià Mercader]

(In reply to comment #13)
> reporter: sorry, the stack trace you provided is useless.
> 
> https://developer.mozilla.org/en/How_to_get_a_stacktrace_for_a_bug_report#Linux
> 
> Please note that if you are using a distro build you need to install their
> symbols. If you're using a 32bit build we provide, crash-reporter should work.
> if you're using your own build, you need --disable-strip and
> --enable-debugger-info-modules/--enable-debug (preferably the latter since
> it'll complain sooner).


I couldn't get a stacktrace yet following those instructions, I will keep trying. I could get the crash ID though:

bp-4bbc9627-acf8-48f1-999f-ebd9e2100811

As for question (1), I'm executing it from a javascript file loaded from a Firefox extension, or also in the Javascript shell of the Extension Developer extension, but it is not part of a XPCOM component, if that was what you were asking for.

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> As for question (1), I'm executing it from a javascript file loaded from a
> Firefox extension, or also in the Javascript shell of the Extension Developer
> extension, but it is not part of a XPCOM component, if that was what you were
> asking for.
This means your global object is a window, which is no good.  If you want to use multithreaded code, you have to do it from a JS component.

Comment 16

[Adrià Mercader]

(In reply to comment #15)

> This means your global object is a window, which is no good.  If you want to
> use multithreaded code, you have to do it from a JS component.

Uops. Can you point me to some resource explaining how to use threads inside XPCOM components?

Does this mean that this is not a bug? In Firefox 3.6.8 works fine for me.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> Uops. Can you point me to some resource explaining how to use threads inside
> XPCOM components?
I don't know if we have any documentation that explains how to make threads inside a component.  It's not different from how you are doing it now; it's just in a JS component.

> Does this mean that this is not a bug? In Firefox 3.6.8 works fine for me.
Seems like this is not a bug and is probably a result of us making the cycle collector error harder when someone is using it off of the main thread.

Comment 18

[Adrià Mercader]

I've run the code inside a JS component and calling bindByName still crashes Firefox when executed from a working Thread, but just in 4.0b4pre, in 3.6.8 works fine.

Crash ID:
127ff231-0f89-5973-32a61960-6799ffd3

Could you please have a look at testcomponent.js and tell me if I haven't done any mistake? It's the first component I've created.

To sum up:
* Calling TestComponent.test_DB() works fine in FF 3.6.8 and 4.0b4pre
* Calling TestComponent.test_thread_DB() works fine in FF 3.6.8 but crashes 4.0b4pre

Comment 19

[Adrià Mercader]

Attachment: Test component to reproduce the crash (run test_thread_DB())

Comment 20

[Adrià Mercader]

Attachment: Stack trace (2nd try)

I've managed to get a stack trace of the crash with some more info (still has some warnings though)

Comment 21

[Shawn Wilsher :sdwilsh]

This feels like it might be a duplicate of bug 464678, but something has clearly changed on trunk that makes us crash now when we didn't in 3.6.  Looks to be a bug in XPConnect.

Comment 22

[Adrià Mercader]

Sorry, I'm a little confused and I would like to clarify this. Is this a bug because:

1- In theory, you can bind parameters to a statement inside a working thread, but right now this is crashing.
or
2- You are not allowed to bind parameters to a statement inside a working thread, but it should throw an exception instead of crashing?

Thanks

Comment 23

[Shawn Wilsher :sdwilsh]

It's (1).  We might be able to work around this by using jsvals instead of nsIVariants...

Comment 24

[Benjamin Smedberg]

We changed the cycle collector to crash (intentionally) if you try to use a cycle collected object on the wrong thread, instead of merely faulting.

Yes, you should use jsval. It's not clear that this needs to block, though. This would only happen with extensions, right?

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to comment #24)
> Yes, you should use jsval. It's not clear that this needs to block, though.
> This would only happen with extensions, right?
Yes.  Not fixing this means it's difficult (but still possible) to bind by name instead of using bind by index.

Comment 26

[Adrià Mercader]

(In reply to comment #25)
> Yes.  Not fixing this means it's difficult (but still possible) to bind by name
> instead of using bind by index.

Just for record, using bindByIndex instead of bindByName also makes Firefox crash.

Comment 27

[Shawn Wilsher :sdwilsh]

(In reply to comment #26)
> Just for record, using bindByIndex instead of bindByName also makes Firefox
> crash.
Right because they both use nsIVariants.  You'd have to use one of these:
http://mxr.mozilla.org/mozilla-central/source/storage/public/mozIStorageBaseStatement.idl#77

Of course, those are deprecated and I was planning on removing them after Firefox 4.0...

Comment 28

[timeless]

Didn't I have a patch to remove that callers of that stuff, which no one had reviewed? :(

Comment 29

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
> Didn't I have a patch to remove that callers of that stuff, which no one had
> reviewed? :(
In various places, yes.  It's still in my long review queue.

Comment 30

[Rares Doghi, Desktop QA]

Hi timeless, can you please take a look at this issue and let us know if it still occurs on your end ? I am unfamiliar with the repro steps and Im not sure if the issue still exists in our latest Firefox builds.

You can find our latest build here: https://www.mozilla.org/en-US/firefox/new/

Seems like a very old issue and if it no longer occurs we can close this bug.

Comment 31

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

I understand this bug to have been about a combination of:

• using mozStorage via XPConnect off the main thread (which worked at one point and then stopped working), which was originally buggy and then fully removed
• lifecycle issues relating to mozStorage's use of XPC variants in the past, since fixed
• possibly legacy add-ons, no longer possible

Marking invalid since all the concerns here are moot.