Closed Bug 765936 Opened 9 years ago Closed 9 years ago

IndexedDB wrapper behavior is still totally broken


(Core :: Storage: IndexedDB, defect)

Not set



Tracking Status
firefox13 --- wontfix
firefox14 + wontfix
firefox15 + fixed
firefox16 + fixed
firefox-esr10 --- unaffected


(Reporter: asuth, Assigned: khuey)



(Keywords: regression, sec-critical, Whiteboard: [advisory-tracking+])


(3 files, 3 obsolete files)

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.
Keywords: sec-critical
Summary: expandos cause ###!!! ASSERTION: Already rooted?!: '!PreservingWrapper()' → IndexedDB wrapper behavior is still totally broken
What versions of FF/geco/b2g are affected?
Attached patch Patch (obsolete) — Splinter Review
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).
Attachment #635499 - Flags: superreview?(peterv)
Attachment #635499 - Flags: review?(bent.mozilla)
Comment on attachment 635499 [details] [diff] [review]

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.
Attachment #635499 - Flags: review?(bent.mozilla) → review+
We can't assert PreservingWrapper anywhere.  Whether or not we're preserving the wrapper is totally independent of everything else that's going on.
Er... we need to somehow assert that we're rooted. Is there no way to do that?
Comment on attachment 635499 [details] [diff] [review]

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.
Attachment #635499 - Flags: superreview?(peterv) → superreview-
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'.
Another option is to make IDBRequest/IDBOpenDBRequest::Create check the argument it passes to SetScriptOwner and bail if it's null.
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.
Attached patch PatchSplinter Review
How about this?  This passes our test suite with no assertions.
Attachment #635499 - Attachment is obsolete: true
Attachment #637274 - Flags: superreview?(peterv)
Attachment #637274 - Flags: review?(bent.mozilla)
Comment on attachment 637274 [details] [diff] [review]

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.
Attachment #637274 - Flags: review?(bent.mozilla) → review+
Kyle, and idea what FF versions are affected by this bug?
Attachment #637274 - Flags: superreview?(peterv) → superreview+
This definitely affects Aurora and Beta.
Closed: 9 years ago
Resolution: --- → FIXED
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.
Attached patch Patch for Aurora (obsolete) — Splinter Review
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.
Attachment #640218 - Flags: approval-mozilla-beta?
Attachment #640218 - Flags: approval-mozilla-aurora?
Attached patch Patch for Beta (obsolete) — Splinter Review
Beta required a slightly different patch.  See above for approval comments.
Attachment #640253 - Flags: approval-mozilla-beta?
Attachment #640218 - Attachment description: Patch for Aurora and Beta → Patch for Aurora
Attachment #640218 - Flags: approval-mozilla-beta?
(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?
This shipped in 13 (see the regressing bug).  ESR doesn't have this issue, but it may have related problems.
Attachment #640218 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #640253 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Kyle Huey [:khuey] ( from comment #23)
> And backed out due to test failures.

When are we going to take another stab at this? We should still be targeting 15, given the risk profile here.
It's on my list for next week.
Attached patch Patch for betaSplinter Review
Same as before, but it actually works.
Attachment #640218 - Attachment is obsolete: true
Attachment #640253 - Attachment is obsolete: true
Attachment #649325 - Flags: review+
Attachment #649325 - Flags: approval-mozilla-beta?
Comment on attachment 649325 [details] [diff] [review]
Patch for beta

approving the patch that works then :)
Attachment #649325 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [advisory-tracking+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.