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

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: foobar.patch

I also removed IDBRequest::Created(IDBCursor.. because it's not used.

Comment 2

[Jan Varga [:janv]]

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 ?

Comment 3

[Andrea Marchesini [:baku]]

> > +  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?

Comment 4

[Jan Varga [:janv]]

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 ?

Comment 5

[Andrea Marchesini [:baku]]

Attachment: foobar.patch

Yep, I see the point.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: foobar.patch

better approach.

Comment 7

[Jan Varga [:janv]]

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.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: foobar.patch

Comment 9

[Andrea Marchesini [:baku]]

Attachment: foobar.patch

Comment 10

[Jan Varga [:janv]]

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.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: foobar.patch

Actually I didn't talk with bholley. Let me ask him a feedback.

Comment 12

[Bobby Holley (:bholley)]

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+

Comment 13

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fec854fceba6

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fec854fceba6