Last Comment Bug 718132 - IndexedDB: Intermittent Failing to get JS wrapper in IDBRequest::NotifyHelperCompleted
: IndexedDB: Intermittent Failing to get JS wrapper in IDBRequest::NotifyHelper...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
Depends on: 720815
Blocks: 674720 722046
  Show dependency treegraph
 
Reported: 2012-01-13 18:20 PST by Gregor Wagner [:gwagner]
Modified: 2012-03-22 11:51 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (85.56 KB, patch)
2012-01-19 11:49 PST, Gregor Wagner [:gwagner]
no flags Details | Diff | Review
Patch, v1 (71.31 KB, patch)
2012-01-20 00:44 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Patch, v1 (75.15 KB, patch)
2012-01-20 07:54 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
khuey: review+
Details | Diff | Review
Crash fix, interdif (9.96 KB, patch)
2012-01-24 01:52 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
jst: review+
Details | Diff | Review

Description Gregor Wagner [:gwagner] 2012-01-13 18:20:25 PST
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) PTO Until July 5th 2012-01-13 18:28:16 PST
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] 2012-01-13 18:33:25 PST
It seems that storing the request object is a temporary fix:

obj = store.put(record);
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-15 13:24:46 PST
(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) PTO Until July 5th 2012-01-15 17:36:15 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 [:khuey] (khuey@mozilla.com) 2012-01-15 22:29:57 PST
(In reply to Jonas Sicking (:sicking) from comment #4)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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) PTO Until July 5th 2012-01-16 22:06:20 PST
And if no expandos are set...
Comment 7 Gregor Wagner [:gwagner] 2012-01-17 10:46:51 PST
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) PTO Until July 5th 2012-01-17 17:00:20 PST
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] 2012-01-19 11:49:56 PST
Created attachment 589930 [details] [diff] [review]
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) PTO Until July 5th 2012-01-19 12:16:57 PST
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!) 2012-01-19 12:43:32 PST
Yeah, already working on it.
Comment 12 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-20 00:44:35 PST
Created attachment 590135 [details] [diff] [review]
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!) 2012-01-20 00:45:18 PST
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!) 2012-01-20 07:54:49 PST
Created attachment 590200 [details] [diff] [review]
Patch, v1

Patch was missing two new files. Now added.
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-20 12:57:23 PST
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 [:khuey] (khuey@mozilla.com) 2012-01-23 05:42:01 PST
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!) 2012-01-23 06:05:19 PST
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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!) 2012-01-23 06:05:48 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/8886b027527a
Comment 20 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-01-24 01:52:56 PST
Created attachment 591037 [details] [diff] [review]
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!) 2012-01-24 02:16:26 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5e83943958
Comment 22 Gregor Wagner [:gwagner] 2012-01-24 10:06:52 PST
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) 2012-01-24 10:44:51 PST
https://hg.mozilla.org/mozilla-central/rev/8b5e83943958
Comment 24 Gregor Wagner [:gwagner] 2012-01-24 10:59:30 PST
(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!

Note You need to log in before you can comment on or make changes to this bug.