Closed Bug 933351 Opened 6 years ago Closed 6 years ago

IDBRequest should not write warning messages about callee when created by IPC IDB

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: baku, Assigned: baku)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 5 obsolete files)

No description provided.
OS: Linux → All
Hardware: x86_64 → All
Summary: IDBRequest should propagate filename and line number when runs on IPC → IDBRequest should not write warning messages about callee when created by IPC IDB
Attached patch foobar.patch (obsolete) — Splinter Review
I also removed IDBRequest::Created(IDBCursor.. because it's not used.
Attachment #825540 - Flags: review?(Jan.Varga)
Component: DOM → DOM: IndexedDB
Comment on attachment 825540 [details] [diff] [review]
foobar.patch

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

First of all, thanks for doing this.

::: dom/indexedDB/IDBFactory.cpp
@@ +206,5 @@
>    factory->mPrivilege = privilege;
>    factory->mDefaultPersistenceType = defaultPersistenceType;
>    factory->mOwningObject = aOwningObject;
>    factory->mContentParent = aContentParent;
> +  factory->mFromIPC = true;

hm, I don't understand this

This method isn't called by ContentParent (created from IPC), is it ?

Also, what about the other case when you have a window ?
> > +  factory->mFromIPC = true;
> 
> hm, I don't understand this
> 
> This method isn't called by ContentParent (created from IPC), is it ?

Yes. This is because it's from IPC. In the other cases mFromIPC will be false.
If false, we retrieve data from the JSContext. Makes sense?
If I'm reading it right, you just don't call CaptureCaller() if the factory is created from the parent actor, but your check is:

if (aFactory->FromIPC()) {
  request->CaptureCaller();
}

IMHO, it's confusing name of the method.

What about this:
if (!aFactory->FromIPC()) {
  request->CaptureCaller();
}

and setting mFromIPC in the other IDBFactory::Create() method ?

Ben ?
Attached patch foobar.patch (obsolete) — Splinter Review
Yep, I see the point.
Attachment #825540 - Attachment is obsolete: true
Attachment #825540 - Flags: review?(Jan.Varga)
Attachment #825855 - Flags: review?(Jan.Varga)
Attached patch foobar.patch (obsolete) — Splinter Review
better approach.
Attachment #825855 - Attachment is obsolete: true
Attachment #825855 - Flags: review?(Jan.Varga)
Attachment #825944 - Flags: review?(Jan.Varga)
Comment on attachment 825944 [details] [diff] [review]
foobar.patch

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

Hm, it seems we can simplify it even more.

Try changing the argument IDBWrapperCache* aOwnerCache to IDBDatabase* aDatabase

and the check:
if (!aDatabase->Factory()->FromIPC()) {
  request->CaptureCaller();
}

It builds fine for me, but I didn't run tests.
Attached patch foobar.patch (obsolete) — Splinter Review
Attachment #825944 - Attachment is obsolete: true
Attachment #825944 - Flags: review?(Jan.Varga)
Attachment #826064 - Flags: review?(Jan.Varga)
Attached patch foobar.patch (obsolete) — Splinter Review
Attachment #826064 - Attachment is obsolete: true
Attachment #826064 - Flags: review?(Jan.Varga)
Attachment #826065 - Flags: review?(Jan.Varga)
Comment on attachment 826065 [details] [diff] [review]
foobar.patch

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

::: dom/indexedDB/IDBDatabase.h
@@ +226,5 @@
> +  IDBFactory*
> +  Factory() const
> +  {
> +    return mFactory;
> +  }

Nit: I wouldn't add it here (WebIDL section), maybe before Info() method declaration.

::: dom/indexedDB/IDBFactory.h
@@ +191,5 @@
> +  bool
> +  FromIPC()
> +  {
> +    return !!mContentParent;
> +  }

Nit: I wouldn't add it here (WebIDL section), maybe after the 2nd SetActor() method declaration.

::: dom/indexedDB/IDBRequest.cpp
@@ -290,5 @@
> -    // If our caller is in another process, we won't have a JSContext on the
> -    // stack, and AutoJSContext will push the SafeJSContext. But that won't have
> -    // any script on it (certainly not after the push), so GetCallingLocation
> -    // will fail when it calls JS_DescribeScriptedCaller. That's fine.
> -    NS_WARNING("Failed to get caller.");

I guess you talked to bholley about this, I think he added this comment.
Attachment #826065 - Flags: review?(Jan.Varga) → review+
Attached patch foobar.patchSplinter Review
Actually I didn't talk with bholley. Let me ask him a feedback.
Attachment #826065 - Attachment is obsolete: true
Attachment #826114 - Flags: feedback?(bobbyholley+bmo)
Comment on attachment 826114 [details] [diff] [review]
foobar.patch

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

Yeah if this can't happen that's fine with me. f+
Attachment #826114 - Flags: feedback?(bobbyholley+bmo) → feedback+
https://hg.mozilla.org/mozilla-central/rev/fec854fceba6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 937006
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.