916094 - IDBFactory::Create() asserts when it's used in JS from chrome in IPC

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Jan Varga [:janv]]

Comment on attachment 804427 [details] [diff] [review]
patch

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

I think we found a better approach, to just expose GetInfoForChrome in QuotaManager.

::: dom/indexedDB/IDBFactory.cpp
@@ +205,5 @@
>    nsCString origin;
>    StoragePrivilege privilege;
>    PersistenceType defaultPersistenceType;
>    nsresult rv =
> +    QuotaManager::GetInfoFromWindowInternal(nullptr, &group, &origin, &privilege,

Nit: that doesn't fit on one line

::: dom/indexedDB/IDBFactory.h
@@ +199,5 @@
>         const Optional<mozilla::dom::StorageType>& aStorageType, bool aDelete,
>         ErrorResult& aRv);
>  
> +  // Used internally when ContentParent calls Create().
> +  static nsresult CreateInternal(JSContext* aCx,

Nit: "static nsresult" on a separate line
I know, the original Create() has it on one line

::: dom/quota/QuotaManager.cpp
@@ +2177,5 @@
> +QuotaManager::GetInfoFromWindowInternal(nsPIDOMWindow* aWindow,
> +                                        nsACString* aGroup,
> +                                        nsACString* aASCIIOrigin,
> +                                        StoragePrivilege* aPrivilege,
> +                                        PersistenceType* aDefaultPersistenceType)

Nit: this doesn't fit on one line, you will have to shift all arguments to the left

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 4

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

Comment on attachment 806676 [details] [diff] [review]
patch

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

Looks good to me!

Comment 5

[Jan Varga [:janv]]

Attachment: patch

Ok, I think I now understand the real problem and here's even cleaner patch IMO.

Comment 6

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

Comment on attachment 806941 [details] [diff] [review]
patch

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

Ok, this looks fine too.

Comment 7

[Jan Varga [:janv]]

Comment on attachment 806676 [details] [diff] [review]
patch

Andrea, go ahead and land it.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 9

[Andrea Marchesini [:baku]]

It's green on try.

Comment 10

[Jan Varga [:janv]]

(In reply to Andrea Marchesini (:baku) from comment #8)
> Created attachment 807073 [details] [diff] [review]
> patch

Andrea, I meant to land the other patch that doesn't add CreateInternal()

Comment 11

[Jan Varga [:janv]]

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

Comment 12

[Ed Morley [:emorley]]

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