718132 - IndexedDB: Intermittent Failing to get JS wrapper in IDBRequest::NotifyHelperCompleted

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Gregor Wagner [:gwagner]]

Putting many objects into the db from a jsm file causes a warning in 
nsresult IDBRequest::NotifyHelperCompleted(HelperBase* aHelper)

The calling code is a jsm which means that we fall into the else-block for if(mScriptContext)

The following 

    obj = GetWrapper();
    NS_ENSURE_TRUE(obj, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

code then bails due to obj being null.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So the fact that this is inside a jsm is likely what is causing problems. The code in the else-block is looking iffy.

First of all it gets the JS-wrapper for the request and uses that when entering the compartment. However the code for normal windows uses the window global to enter the compartment. Is it really ok to use the wrapper for the request?

Second, calling GetWrapper can return null if we've GC'ed the js-wrapper, no? That seems like a legitimate case and not something that should cause us to error out. Especially if we don't root the wrapper anywhere.

Bent, do you know what the right thing to do here is?

Comment 2

[Gregor Wagner [:gwagner]]

It seems that storing the request object is a temporary fix:

obj = store.put(record);

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Jonas Sicking (:sicking) from comment #1)
> Second, calling GetWrapper can return null if we've GC'ed the js-wrapper,
> no? That seems like a legitimate case and not something that should cause us
> to error out. Especially if we don't root the wrapper anywhere.

The request roots its own wrapper though, doesn't it?

(In reply to Gregor Wagner [:gwagner] from comment #2)
> It seems that storing the request object is a temporary fix:
> 
> obj = store.put(record);

Presumably this manual wrapper preservation causes it to work.  The question is why the wrapper isn't preserved to begin with ...

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> The request roots its own wrapper though, doesn't it?

Do we do that even if no event handlers are registered?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Jonas Sicking (:sicking) from comment #4)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> > The request roots its own wrapper though, doesn't it?
> 
> Do we do that even if no event handlers are registered?

Aha, good question.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

And if no expandos are set...

Comment 7

[Gregor Wagner [:gwagner]]

I hit the same warning again even with my rooting workaround. It seems to be less common but still existing.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How are you rooting the object? Just creating a local variable pointing to the return value will just keep it rooted until the function exits. If you keep an array on the transaction, and add all requests to that array, you should be rooting properly.

Comment 9

[Gregor Wagner [:gwagner]]

Attachment: Testcase

Just run the test in dom/contacts/tests/ 

You might have to run it a few times but I increased the number of contacts I create to 800 and after some time you should see:
147 INFO TEST-PASS | /tests/dom/contacts/tests/test_contacts_basics.html | The contact now has an ID.
WARNING: NS_ENSURE_TRUE(obj) failed: file /Volumes/mac/moz/ws1/dom/indexedDB/IDBRequest.cpp, line 155
WARNING: NS_ENSURE_TRUE(obj) failed: file /Volumes/mac/moz/ws1/dom/indexedDB/IDBRequest.cpp, line 155

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ben, is this something you could have a look at? This seems like a blocker for using IndexedDB in jsm

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, already working on it.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Gregor, please give this patch a try!

Blake, this changes the way we do wrappers for indexedDB, take a look and see what you think?

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 590135 [details] [diff] [review]
Patch, v1

Peter, you could take a look too maybe? :)

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Patch was missing two new files. Now added.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 590200 [details] [diff] [review]
Patch, v1

bent asked me to take a look at this, setting the flag so I don't forget.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 590200 [details] [diff] [review]
Patch, v1

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

::: content/base/src/nsContentUtils.cpp
@@ +5436,5 @@
>    NS_IMETHOD_(void) NoteScriptChild(PRUint32 langID, void* child)
>    {
> +    if (langID == nsIProgrammingLanguage::JAVASCRIPT &&
> +        child == mWrapper) {
> +      mFound = true;

This is a behavior change, do we want this?

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #16)
> This is a behavior change, do we want this?

Yep, the old code assumed only one script child per preserved wrapper, which is not always true.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8886b027527a

Comment 19

[Ed Morley [:emorley]]

Backed out of inbound for xpcshell failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=8886b027527a

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa713a50f435

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Crash fix, interdif

This fixes the crash, which was a simple refcounting bug due to NS_DROP_JS_OBJECTS in IDBFactory.

I also removed some of the preserved wrapper stuff because I think we don't really need it after all.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5e83943958

Comment 22

[Gregor Wagner [:gwagner]]

I tried my patch on top of both patches and got the same error again right away:
WARNING: NS_ENSURE_TRUE(obj) failed: file /Users/idefix/moz/ws5/dom/indexedDB/IDBRequest.cpp, line 155

Comment 23

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/8b5e83943958

Comment 24

[Gregor Wagner [:gwagner]]

(In reply to Gregor Wagner [:gwagner] from comment #22)
> I tried my patch on top of both patches and got the same error again right
> away:
> WARNING: NS_ENSURE_TRUE(obj) failed: file
> /Users/idefix/moz/ws5/dom/indexedDB/IDBRequest.cpp, line 155

Ok this was my mistake. Works fine!