Closed Bug 1198093 Opened 9 years ago Closed 8 years ago

Expose indexedDB to System with [Exposed=System]

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox43 --- affected
firefox48 --- fixed

People

(Reporter: reuben, Assigned: bevis, Mentored)

Details

(Whiteboard: [tw-dom] btpp-active)

Attachments

(2 files, 5 obsolete files)

      No description provided.
I'd like to take this bug to be more familiar with IndexedDB-related implementation.
Assignee: nobody → btseng
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #3)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=75b69eaa97e7

Much better then the 1st WIP in comment 2 (with lots of devtools failures), after adopting dom::SystemGlobalResolve() and dom::SystemGlobalEnumerate() in sandbox_resolve() and sandbox_enumerate() respectively to import all the interfaces with [Exposed=System] into Sandbox.

However, |test_locale_aware_indexes.js| consistently failed in *child-process* when the index option of locale is "auto":
TEST-UNEXPECTED-FAIL | xpcshell-child-process.ini:dom/indexedDB/test/unit/test_locale_aware_indexes.js | is - [is : 17] "en-US" == "en_US"

Keep figuring out the root cause.
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #4)
> However, |test_locale_aware_indexes.js| consistently failed in
> *child-process* when the index option of locale is "auto":
> TEST-UNEXPECTED-FAIL |
> xpcshell-child-process.ini:dom/indexedDB/test/unit/test_locale_aware_indexes.
> js | is - [is : 17] "en-US" == "en_US"
> 
> Keep figuring out the root cause.

It shall be "en_US" instead of "en-US" in IndexedDatabaseManager::Init() when mLocale.IsEmpty()[1] according to Level 2 canonicalization[2].

But we still have to figure out why the behaviour is changed after applied WIP in comment 3.

[1] https://hg.mozilla.org/mozilla-central/annotate/dd1abe874252e507b825a0a4e1063b0e13578288/dom/indexedDB/IndexedDatabaseManager.cpp#l396
[2] http://userguide.icu-project.org/locale
Whiteboard: [tw-dom] → [tw-dom] btpp-active
With the WIP in comment 6 applied, there are 2 issues remained according to the try server result in comment 3:
1. (xpcshell-child-process, m-e10s) test_locale_aware_indexes.js | is - [is : 17] "en-US" == "en_US" in child process. 
2. (w-e10s) navigatorlanguage.html | NavigatorLanguage: the most preferred language is the one returned by navigator.language - assert_equals: navigator.languages is the most preferred language first expected (string) "" but got (undefined) undefined.

After further investigation, I found that, in child process, we are not able to retrieve the value of "intl.accept_languages" from preference service in child process.[1][2] This is out of my understanding of how the [Expose=System] impacts to. :(

Kyle, would you mind providing some hint on this? :p Thanks!

[1] https://hg.mozilla.org/mozilla-central/annotate/d1d47ba19ce9d46222030d491f9fe28dbf80be12/dom/indexedDB/IndexedDatabaseManager.cpp#l381
[2] https://hg.mozilla.org/mozilla-central/annotate/d1d47ba19ce9d46222030d491f9fe28dbf80be12/dom/base/Navigator.cpp#l453
Flags: needinfo?(khuey)
I dug into #1, which turned out to be quite insane.  Basically we're in the content process, and starting the IndexedDatabaseManager before we receive the value of the intl.accept_languages preference.  Starting IndexedDatabaseManager was moved earlier because before this change the IndexedDB properties were not available on all System scopes (you have to specifically request it via Cu.importGlobalProperties) but now they are, and ctypes.jsm *enumerates* all the properties at startup.
Flags: needinfo?(khuey)
I think your best path forward is to make ExperimentalFeaturesEnabled return true automatically if we're getting called on a System global.

[15:32:16] <khuey> bz: if I'm in a Func can I tell what type of scope I am being created for easily?
[15:32:20] <khuey> bz: e.g. Window, System, etc?
[15:32:34] <bz> From the obj passed in
[15:34:14] <khuey> bz: right ... but is there an easy way to do that
[15:35:39]  * bz looks it up
[15:36:34] <khuey> bz: context https://bugzilla.mozilla.org/show_bug.cgi?id=1198093#c8
[15:36:38] <firebot> Bug 1198093 — NEW, btseng@mozilla.com — Use [Exposed=System] instead of
                     IndexedDatabaseManager::DefineIndexedDB
[15:37:18] <bz> ok
[15:37:37] <bz> So what we do for our actual exposure checks is...
[15:37:48] <bz> 1)  Get the actual global via js::GetGlobalForObjectCrossCompartment
[15:38:05] <bz> (I think this mostly matters for navigator, but not sure offhand)
[15:38:06] <khuey> what I would like to do is short-circuit the exposure check (inside our
                   function) if we have a System global
[15:38:21] <bz> 2) Call
                http://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingUtils.cpp#2568
[15:38:23] <khuey> so I don't have to create IndexedDatabaseManager that early
[15:38:36] <bz> So basically check whether the global's class name is "BackstagePass"
[15:38:39] <bz> in your case

This is ugly but it should work.  You can do 

if (IsNonExposedGlobal(cx, js::GetGlobalForObjectCrossCompartment(obj), GlobalNames::BackstagePass)) {
  return true;
}

without creating the IndexedDatabaseManager.  Yuck :P  Please add comments.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #9)
> I think your best path forward is to make ExperimentalFeaturesEnabled return
> true automatically if we're getting called on a System global.
Thanks for your advice.
The failed test cases in comment 7 are passed with this fix.

However, I still want to spend more time offline to understand the background of this fix before adding this solution into the patch:
1. It's still wired to me that having IndexedDatabaseManager created too early will impact the result of "Preferences::GetLocalizedString("intl.accept_languages")" in Navigator::GetAcceptLanguages(). Is there some sort of cache in Preference that cause this problem?
2. Figuring out how ctypes.jsm *enumerates* all the properties at startup.
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #10)
> However, I still want to spend more time offline to understand the
> background of this fix before adding this solution into the patch:
> 1. It's still wired to me that having IndexedDatabaseManager created too
> early will impact the result of
> "Preferences::GetLocalizedString("intl.accept_languages")" in
> Navigator::GetAcceptLanguages(). Is there some sort of cache in Preference
> that cause this problem?
> 2. Figuring out how ctypes.jsm *enumerates* all the properties at startup.

I would suggest you set a breakpoint where IndexedDatabaseManager gets the value of intl.accept_languages and then run with and without your patch.  You can look at the callstack to see what is causing us to start the IndexedDatabaseManager, and trace it through the Preferences::GetLocalizedString call to see what's going wrong.  You should end up in nsChromeRegistryContent, and also look at when we're hitting ContentChild::RecvRegisterChrome.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12)
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #10)
> > However, I still want to spend more time offline to understand the
> > background of this fix before adding this solution into the patch:
> > 1. It's still wired to me that having IndexedDatabaseManager created too
> > early will impact the result of
> > "Preferences::GetLocalizedString("intl.accept_languages")" in
> > Navigator::GetAcceptLanguages(). Is there some sort of cache in Preference
> > that cause this problem?
> > 2. Figuring out how ctypes.jsm *enumerates* all the properties at startup.
> 
> I would suggest you set a breakpoint where IndexedDatabaseManager gets the
> value of intl.accept_languages and then run with and without your patch. 
> You can look at the callstack to see what is causing us to start the
> IndexedDatabaseManager, and trace it through the
> Preferences::GetLocalizedString call to see what's going wrong.  You should
> end up in nsChromeRegistryContent, and also look at when we're hitting
> ContentChild::RecvRegisterChrome.

Thanks for your advices. :)

After further investigation, In child process,
1. All interface/properties with [Exposed=System] will be validated in XRE_InitChildProcess() when creating XPCJSRuntime and Sandbox object, right after the main function of the child process is executed [1].
2. However, the chrome package that contains the default property of "intl.accept-language" is installed to the child process after the child process is executed [2].
That's the reason why "intl.accept-language" is failed to be retrieved in child process.
3. The reason why Navigator::GetAcceptLanguages() was impacted is that
   a. There is a cache in nsStringBundleService [3].
   b. 2nd attempt of the same nsStringBundle instance will always be failed if it was failed load at 1st time [4].
   c. Since "intl.accept-language" is always failed to be retrieved by IDB Manager, it's expected failed in Navigator::GetAcceptLanguages() if the same nsStringBundle is available in the cache.

[1] https://hg.mozilla.org/mozilla-central/annotate/f14898695ee0dd14615914f3e1401f17df57fdd7/ipc/contentproc/plugin-container.cpp#l237
[2] https://hg.mozilla.org/mozilla-central/annotate/f14898695ee0dd14615914f3e1401f17df57fdd7/dom/ipc/ContentParent.cpp#l2517
[3] https://hg.mozilla.org/mozilla-central/annotate/f14898695ee0dd14615914f3e1401f17df57fdd7/intl/strres/nsStringBundle.cpp#l585
[4] https://hg.mozilla.org/mozilla-central/annotate/f14898695ee0dd14615914f3e1401f17df57fdd7/intl/strres/nsStringBundle.cpp#l64
The next action is trying to isolate "dom.indexedDB.experimental" from IndexedDatabaseManager::Init() to see if we can have [Expose=System] without the workaround in comment 9.
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #4)
> after adopting dom::SystemGlobalResolve() and dom::SystemGlobalEnumerate()
> in sandbox_resolve() and sandbox_enumerate() respectively to import all the
> interfaces with [Exposed=System] into Sandbox.

Is this OK? Sandbox should expose a global property only if it is explicitly requested by wantGlobalProperties, no?
(In reply to Masatoshi Kimura [:emk] from comment #17)
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #4)
> > after adopting dom::SystemGlobalResolve() and dom::SystemGlobalEnumerate()
> > in sandbox_resolve() and sandbox_enumerate() respectively to import all the
> > interfaces with [Exposed=System] into Sandbox.
> 
> Is this OK? Sandbox should expose a global property only if it is explicitly
> requested by wantGlobalProperties, no?

That's a good point, thanks!

I'll revise my patch to address this but it also implies that IndexedDatabaseManager::DefineIndexedDB() is still required by Sandbox and should not be deprecated.
I'd like to rise my concern to add [Exposed=System] for IDB-related interfaces(I should rise earlier before all this changes. :-( ):
1. As mentioned by :emk in comment 17, it seems not okay to have dom::SystemGlobalResolve() in sandbox_resolve() because the interfaces exposed to System scope are not requested by the owner of the sandbox.
2. Similar issues for the scripts running in system scope after exposed IDB interfaces into system:
  - All the IDB-related prototypes will be available automatically.
  - However, it's meaningless unless global property of "indexedDB" is imported.
3. IMHO, [Exposed=System] is useful for the interfaces that include constructors or some static methods/properties. There seems not much benefit if we adopt this for "indexedDB". :-(
4. In addition, to meet 1), IndexedDatabaseManager::DefineIndexedDB() is still required for Sandbox.


With these concern, do we still prefer to specify [Exposed=System] to these IDB interfaces?

If yes, may I have your feedback of this WIP to see if this is the right approach?
Here is the summary of the change in this WIP:
1. Unlike "intl.accept_languages", preferences defined in all.js works fine, so Preferences::GetBool(kPrefExperimental) is retrieved directly in IndexedDatabaseManager::ExperimentalFeaturesEnabled() at 1st run for BackstagePass scope.
2. To ensure that IDB interfaces won't be imported into sandbox automatically, 
   2 methods are defined for BackstagePass & Sandbox to import global properties accordingly instead of using |xpc::GlobalProperties::Define()|, since interfaces with [Exposed=System] will be imported automatically into BackstagePass.

Thanks!
Attachment #8733822 - Flags: feedback?(khuey)
I think we should still do this even if we do continue to need the other path for sandboxes.  I'll try to take a look at this tomorrow.
IMO two separate methods are unnecessary. It is harmless to call IDB*::GetConstructorObject() on BackstagePass.
(In reply to Masatoshi Kimura [:emk] from comment #23)
> IMO two separate methods are unnecessary. It is harmless to call
> IDB*::GetConstructorObject() on BackstagePass.

Yes, but it's more clear to identify the difference between BackstagePass & Sandbox. :)
Comment on attachment 8733822 [details] [diff] [review]
WIP(v2): Use [Exposed=System] for IDB-Related Interfaces.

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

::: js/xpconnect/src/Sandbox.cpp
@@ +1008,5 @@
> +xpc::GlobalProperties::DefineInXPCComponents(JSContext* cx, JS::HandleObject obj)
> +{
> +    if (indexedDB &&
> +        !IndexedDatabaseManager::DefineIndexedDB(cx, obj))
> +        return false;

I don't understand why this is necessary.  The Exposed=System stuff should do this automatically, no?
Attachment #8733822 - Flags: feedback?(khuey) → feedback-
indexDB is not an interface but an attribute of the global object. I don't know whether [Exposed=System] can mark an attribute, but BackstagePass is not migrated to WebIDL yet anyway.
Does this answer your question?
(In reply to Masatoshi Kimura [:emk] from comment #26)
> indexDB is not an interface but an attribute of the global object. I don't
> know whether [Exposed=System] can mark an attribute, but BackstagePass is
> not migrated to WebIDL yet anyway.
> Does this answer your question?

Ah, ok, I see.
And yes, you can use [Exposed=System] on an attribute, but since (as you point out) BackstagePass is not a WebIDL global ...
Comment on attachment 8733822 [details] [diff] [review]
WIP(v2): Use [Exposed=System] for IDB-Related Interfaces.

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

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +398,1 @@
>    }

Why is this necessary?

@@ +711,5 @@
> +  // In child process, properties will be enumerated immediately at startup
> +  // before default property of |intl.accept_languages| is ready.
> +  // Since the only dependency of this function is |gExperimentalFeaturesEnabled|,
> +  // we retrieve gExperimentalFeaturesEnabled without IndexedDatabaseManager::Init()
> +  // in BackstagePass scope.

I'd write this as:

If, in the child process, properties of the global object are enumerated before the chrome registry (and thus the value of |intl.accept_languages| is ready, calling IndexedDatabaseManager::Init will permanently break that preference. We can retrieve gExperimentalFeatures enabled without actually going through IndexedDatabaseManager though.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #27)
> (In reply to Masatoshi Kimura [:emk] from comment #26)
> > indexDB is not an interface but an attribute of the global object. I don't
> > know whether [Exposed=System] can mark an attribute, but BackstagePass is
> > not migrated to WebIDL yet anyway.
> > Does this answer your question?
> 
> Ah, ok, I see.
:emk, thanks for clarification.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #28)
> And yes, you can use [Exposed=System] on an attribute, but since (as you
> point out) BackstagePass is not a WebIDL global ...

Yes, that's the reason I still have to define this property internally in Sandbox.cpp.
Comment on attachment 8733822 [details] [diff] [review]
WIP(v2): Use [Exposed=System] for IDB-Related Interfaces.

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

set feedback? again with following explanation to see we are all good to move on.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +398,1 @@
>    }

Sorry for not explaining this while attaching the patch. :p
This is a minor issue caught by |test_locale_aware_indexes.js| when the preference of |intl.accept_languages| is not accessible in child process. (In short, we set the default value in wrong format.)
The reason is explained in comment 5.
I attached it here again for your reference.

I can file another bug instead if it is not proper to include it in this bug in another patch file. :)

--
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #4)
> However, |test_locale_aware_indexes.js| consistently failed in
> *child-process* when the index option of locale is "auto":
> TEST-UNEXPECTED-FAIL |
> xpcshell-child-process.ini:dom/indexedDB/test/unit/test_locale_aware_indexes.
> js | is - [is : 17] "en-US" == "en_US"
> 

It shall be "en_US" instead of "en-US" in IndexedDatabaseManager::Init() when mLocale.IsEmpty()[1] according to Level 2 canonicalization[2].

[1] https://hg.mozilla.org/mozilla-central/annotate/dd1abe874252e507b825a0a4e1063b0e13578288/dom/indexedDB/IndexedDatabaseManager.cpp#l396
[2] http://userguide.icu-project.org/locale

@@ +711,5 @@
> +  // In child process, properties will be enumerated immediately at startup
> +  // before default property of |intl.accept_languages| is ready.
> +  // Since the only dependency of this function is |gExperimentalFeaturesEnabled|,
> +  // we retrieve gExperimentalFeaturesEnabled without IndexedDatabaseManager::Init()
> +  // in BackstagePass scope.

Thanks for you revise. It's more precise to what I am trying to do. :)

::: js/xpconnect/src/Sandbox.cpp
@@ +1008,5 @@
> +xpc::GlobalProperties::DefineInXPCComponents(JSContext* cx, JS::HandleObject obj)
> +{
> +    if (indexedDB &&
> +        !IndexedDatabaseManager::DefineIndexedDB(cx, obj))
> +        return false;

Unfortunately, as explained in comment 26, BackstagePass is not a webidl global. :(
We still need this for the use case of Cu.importGlobalProperties(["indexedDB"]).
Attachment #8733822 - Flags: feedback- → feedback?(khuey)
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #31)
> ::: dom/indexedDB/IndexedDatabaseManager.cpp
> @@ +398,1 @@
> >    }
> 
> Sorry for not explaining this while attaching the patch. :p
> This is a minor issue caught by |test_locale_aware_indexes.js| when the
> preference of |intl.accept_languages| is not accessible in child process.
> (In short, we set the default value in wrong format.)
> The reason is explained in comment 5.
> I attached it here again for your reference.
> 
> I can file another bug instead if it is not proper to include it in this bug
> in another patch file. :)

We either need to delay the test until the value of intl.accept_languages is available or just skip this test in this configuration.
Comment on attachment 8733822 [details] [diff] [review]
WIP(v2): Use [Exposed=System] for IDB-Related Interfaces.

I think there's just a couple small changes to make from the past few comments and this will be good to go.
Attachment #8733822 - Flags: feedback?(khuey)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32)
> >  mLocale.AssignLiteral("en_US");
> We either need to delay the test until the value of intl.accept_languages is
> available or just skip this test in this configuration.
No. Sorry for misleading. :(
This test case was failed before we come out a solution with :bz in IndexedDatabaseManager::ExperimentalFeaturesEnabled() in comment 9.

This test can be run correctly if the WIP patch applied, because we already ensure that IndexedDatabaseManager:Init() will be triggered after the chrome package containing the default property of "intl.accept-language" is installed to the child process.

This line is never reached if |intl.accept-language| is always available.
I'd like to fix this line just because it's annoying to me after realizing the locale format defined by ICU and "en_US" is the expected result in test_locale_aware_indexes.js. :p
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #33)
> Comment on attachment 8733822 [details] [diff] [review]
> WIP(v2): Use [Exposed=System] for IDB-Related Interfaces.
> 
> I think there's just a couple small changes to make from the past few
> comments and this will be good to go.

Thanks, I'll provide formal patches for the review later!
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #34)
> This line is never reached if |intl.accept-language| is always available.
> I'd like to fix this line just because it's annoying to me after realizing
> the locale format defined by ICU and "en_US" is the expected result in
> test_locale_aware_indexes.js. :p
And icu::Locale::createCanonical() is used to parse the "intl.accept_languages" if available in IndexedDatabaseManager::Init()! [1]

[1] https://hg.mozilla.org/mozilla-central/annotate/63be002b4a803df1122823841ef7633b7561d873/dom/indexedDB/IndexedDatabaseManager.cpp#l388
Attached patch (v1) Part1: WebIDL Change. (obsolete) — Splinter Review
Formal patch part1 with WebIDL change.
Attachment #8730089 - Attachment is obsolete: true
Attachment #8733822 - Attachment is obsolete: true
Attachment #8736205 - Flags: review?(khuey)
Implementation change.
Attachment #8736207 - Flags: review?(khuey)
Bug can be fixed without this patch but it's nice to have since IDBManager::mLocale is formed with icu::Locale::createCanonical() in IDBManager::Init() when |intl.accept_languages| is available.
Attachment #8736212 - Flags: review?(khuey)
Comment on attachment 8736205 [details] [diff] [review]
(v1) Part1: WebIDL Change.

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

I know y'all often split out the WebIDL and implementation changes when working on Firefox OS, but we generally don't do that here unless the interfaces themselves need specific review.  Please squash this and part 2 together when landing.
Attachment #8736205 - Flags: review?(khuey) → review+
Comment on attachment 8736207 [details] [diff] [review]
(v1) Part2: Implementation Change.

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

::: js/xpconnect/src/Sandbox.cpp
@@ +941,5 @@
>  bool
>  xpc::GlobalProperties::Define(JSContext* cx, JS::HandleObject obj)
>  {
> +    // Properties could be exposed to into System automatically but not into
> +    // Sandbox with |[Exposed=System]| specified in WebIDL definition.

Properties will be exposed to System automatically but not to Sandboxes if |[Exposed=System]| is specified.

@@ +944,5 @@
> +    // Properties could be exposed to into System automatically but not into
> +    // Sandbox with |[Exposed=System]| specified in WebIDL definition.
> +    // This function is a holder of common properties not exposed
> +    // automatically but requested either in |Cu.importGlobalProperties| or
> +    // |wantGlobalProperties| of a sandbox. See Bug 1198093 for detailed info.

This function holds common ... but that can be requested

No need for a bug # here.
Attachment #8736207 - Flags: review?(khuey) → review+
1. squash patch part1 and part 2.
2. address comment 42.
Attachment #8736205 - Attachment is obsolete: true
Attachment #8736207 - Attachment is obsolete: true
Attachment #8737049 - Flags: review+
update patch description.
Attachment #8736212 - Attachment is obsolete: true
Attachment #8737050 - Flags: review+
IndexedDatabaseManager::DefineIndexedDB is still necessary after this fix to export the "indexedDB" as one property of the global object per comment 20 and comment 26.
Summary: Use [Exposed=System] instead of IndexedDatabaseManager::DefineIndexedDB → Expose indexedDB to System with [Exposed=System]
try server result is attached in comment 43.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/57f7f132083c
https://hg.mozilla.org/mozilla-central/rev/7fa6eea202cb
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.