765936 - IndexedDB wrapper behavior is still totally broken

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

Attachment: a test that adds an expando and dies

I accidentally added an expando to a request because meta-/ thought onfailure was a good name and I agreed.

This results in:
###!!! ASSERTION: Already rooted?!: '!PreservingWrapper()', file /home/visbrero/rev_control/fgit/b2g-gaia/mozilla-central/dom/indexedDB/IDBRequest.cpp, line 80

I have included a dubiously named xpcshell test for your edification.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

What versions of FF/geco/b2g are affected?

Comment 2

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

Attachment: Patch

The assertion is bogus, but there is another problem here.  Since bug 731227 we call NS_HOLD_JS_OBJECTS on all requests at creation time.  All the existing rooting/unrooting stuff just serves to duplicate that, except that we can run into scenarios where we hold twice and then call drop (and then end up with an unpreserved wrapper).

Comment 3

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

Comment on attachment 635499 [details] [diff] [review]
Patch

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

::: dom/indexedDB/IDBRequest.cpp
@@ -110,5 @@
>  
>    JSAutoRequest ar(cx);
>    JSAutoEnterCompartment ac;
>    if (ac.enter(cx, global)) {
> -    RootResultVal();

I think we should assert PreservingWrapper... somewhere? At the top of this method most likely.

Comment 4

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

We can't assert PreservingWrapper anywhere.  Whether or not we're preserving the wrapper is totally independent of everything else that's going on.

Comment 5

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

Er... we need to somehow assert that we're rooted. Is there no way to do that?

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 635499 [details] [diff] [review]
Patch

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

I don't think we can rely on SetScriptOwner unless it returns false if a null aScriptOwner is passed in.

Comment 7

[Peter Van der Beken [:peterv]]

Not sure we can do that, given other consumers of that API. The other option would be to somehow have one flag to signal 'rooted by SetScriptOwner' and 'rooted by RootResultVal'.

Comment 8

[Peter Van der Beken [:peterv]]

Another option is to make IDBRequest/IDBOpenDBRequest::Create check the argument it passes to SetScriptOwner and bail if it's null.

Comment 9

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

I was going to say that I don't think the script owner should ever be null, but now with the IPC stuff I'm not so sure.  I'll dig further.

Comment 10

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

Attachment: Patch

How about this?  This passes our test suite with no assertions.

Comment 11

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

Comment on attachment 637274 [details] [diff] [review]
Patch

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

Looks good to me with these things addressed:

::: dom/indexedDB/IDBFactory.cpp
@@ +408,5 @@
>  
>    if (mWindow) {
>      window = mWindow;
> +    scriptOwner =
> +      static_cast<nsGlobalWindow*>(window.get())->FastGetGlobalJSObject();

I don't like this... We're not in performance-critical code here, can we just use the nsI* methods to get this?

::: dom/indexedDB/IDBWrapperCache.cpp
@@ +47,5 @@
>  
>  bool
>  IDBWrapperCache::SetScriptOwner(JSObject* aScriptOwner)
>  {
> +  MOZ_ASSERT(aScriptOwner);

NS_ASSERTION. We'll do a full replace someday, but for now please use the other. (I'm still a little miffed that we don't get stacks with MOZ_ASSERT)

@@ +68,5 @@
> +#ifdef DEBUG
> +void
> +IDBWrapperCache::AssertIsRooted() const
> +{
> +  MOZ_ASSERT(nsContentUtils::AreJSObjectsHeld(const_cast<IDBWrapperCache*>(this)),

Here too.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Kyle, and idea what FF versions are affected by this bug?

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/03c5347327d8

Comment 14

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

This definitely affects Aurora and Beta.

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/03c5347327d8

Comment 16

[Alex Keybl [:akeybl]]

What's the risk profile of this bug? Please nominate for Aurora approval, and Beta approval if low risk or a particularly critical security regression.

Comment 17

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

Attachment: Patch for Aurora

This is a narrower patch for Aurora and Beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 731227
User impact if declined: A potential SG crit involving stale pointers to GCd objects.  I haven't actually constructed a test case which crashes though.
Testing completed (on m-c, etc.): Has been on m-c for a bit, is simple enough to understand.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None.

Comment 18

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

Attachment: Patch for Beta

Beta required a slightly different patch.  See above for approval comments.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

(In reply to David Bolter [:davidb] from comment #12)
> Kyle, and idea what FF versions are affected by this bug?

Any answer to this? Did we ship this problem in a final release or is it a more recent regression? Is Firefox 13 affected or existing ESR?

Comment 20

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

This shipped in 13 (see the regressing bug).  ESR doesn't have this issue, but it may have related problems.

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 640253 [details] [diff] [review]
Patch for Beta

[Triage Comment]
Spoke with Dan/Al - although this is low risk, we're already taking significant change in our final beta, and we'll try to mitigate by taking less change that's on the line.

Comment 22

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/ddda87bd4a4a

Comment 23

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

And backed out due to test failures.

https://hg.mozilla.org/releases/mozilla-aurora/rev/626ac9c0bf97

Comment 24

[Alex Keybl [:akeybl]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #23)
> And backed out due to test failures.
> 
> https://hg.mozilla.org/releases/mozilla-aurora/rev/626ac9c0bf97

When are we going to take another stab at this? We should still be targeting 15, given the risk profile here.

Comment 25

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

It's on my list for next week.

Comment 26

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

Attachment: Patch for beta

Same as before, but it actually works.

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 649325 [details] [diff] [review]
Patch for beta

approving the patch that works then :)

Comment 28

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

http://hg.mozilla.org/releases/mozilla-beta/rev/5c49f5a87e9e