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

RESOLVED FIXED

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gwagner, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 674720
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?
(Reporter)

Comment 2

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

obj = store.put(record);
(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 ...
(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?
(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.
And if no expandos are set...
(Reporter)

Comment 7

6 years ago
I hit the same warning again even with my rooting workaround. It seems to be less common but still existing.
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.
(Reporter)

Comment 9

6 years ago
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
Ben, is this something you could have a look at? This seems like a blocker for using IndexedDB in jsm
Yeah, already working on it.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
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?
Attachment #590135 - Flags: review?(mrbkap)
Comment on attachment 590135 [details] [diff] [review]
Patch, v1

Peter, you could take a look too maybe? :)
Attachment #590135 - Flags: review?(peterv)
Created attachment 590200 [details] [diff] [review]
Patch, v1

Patch was missing two new files. Now added.
Attachment #590135 - Attachment is obsolete: true
Attachment #590135 - Flags: review?(peterv)
Attachment #590135 - Flags: review?(mrbkap)
Attachment #590200 - Flags: review?(peterv)
Attachment #590200 - Flags: review?(mrbkap)
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.
Attachment #590200 - Flags: review?(khuey)
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?
Attachment #590200 - Flags: review?(khuey) → review+
Attachment #590200 - Flags: review?(peterv)
Attachment #590200 - Flags: review?(mrbkap)
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8886b027527a
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
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.
Attachment #591037 - Flags: review?(jst)

Updated

6 years ago
Attachment #591037 - Flags: review?(jst) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b5e83943958
(Reporter)

Comment 22

6 years ago
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
https://hg.mozilla.org/mozilla-central/rev/8b5e83943958
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Reporter)

Comment 24

6 years ago
(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!
Depends on: 720815
Blocks: 722046
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla12 → ---
You need to log in before you can comment on or make changes to this bug.