1097479 - [BrowserAPI] Allow embed remote apps or widgets in content process if nested-oop is enabled

Status: RESOLVED FIXED

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

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Embedding apps in content process is disabled in bug 1059662. We should allow the content to embed apps if

 1. Nested-oop is enabled
 2. The to be embedded <iframe> is remote=true

Comment 1

[Kershaw Chang [:kershaw]]

Attachment: Allow nested content process to embed apps

Hi Kan-Ru,

Could you take a look at this patch?

Thanks.

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8538353 [details] [diff] [review]
Allow nested content process to embed apps

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

::: dom/html/nsGenericHTMLFrameElement.cpp
@@ +442,5 @@
>    if (!nsIMozBrowserFrame::GetReallyIsBrowserOrApp()) {
>      return NS_OK;
>    }
>  
> +  // Only nested content process is allowed to embed an app.

// Only allow content process to embed an app when nested content process is enabled.

@@ +445,5 @@
>  
> +  // Only nested content process is allowed to embed an app.
> +  if (XRE_GetProcessType() != GeckoProcessType_Default &&
> +      !GetBoolAttr(nsGkAtoms::Remote) &&
> +      !Preferences::GetBool("dom.ipc.tabs.nested.enabled", false)) {

Should avoid this Preferences check by Preference cache. You could find examples in the same file like BrowserFramesEnabled() or WidgetsEnabled()

The test should be

 IsContentProcess || !(HasRemote && NestedEnabled)

Comment 3

[Kevin Chen[:kechen] (UTC + 8)]

Hi Kershaw,I would like to practice fixing some bugs. Is it okay that I take this bug? Thanks

Comment 4

[Kershaw Chang [:kershaw]]

Sure. Feel free to do this.

Comment 5

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Allow nested content process to embed apps

Hi Kershaw, could you help me to check this patch? thank you.

Comment 6

[Kershaw Chang [:kershaw]]

Comment on attachment 8541973 [details] [diff] [review]
Allow nested content process to embed apps

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

Looks like your patch is mixed with something else.
Please be more careful at next time.

In addition, our commit messages should look like this:
Bug <number> - <short description>.
The description you give to the patch should generally try to describe what the patch is accomplishing, and is usually different from just the bug's title.

::: dom/html/nsGenericHTMLFrameElement.cpp
@@ +444,5 @@
>    }
>  
> +  // Only nested content process is allowed to embed an app.
> +  if (XRE_GetProcessType() != GeckoProcessType_Default &&
> +      !GetBoolAttr(nsGkAtoms::Remote) &&

Please modify the comment as Kan-Ru's feedback.
We also need to add a new function to check the preference. Please check BrowserFramesEnabled() or WidgetsEnabled() in the same file.

Comment 7

[Kershaw Chang [:kershaw]]

BTW, please also make the old patches obsolete when you upload a new one.
Thanks.

Comment 8

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Allow nested content process to embed apps and add a nested pref check function

Hi Kershaw, I've made some modify to the patch, could you help me to check this again? Thank you.

Comment 9

[Kershaw Chang [:kershaw]]

Comment on attachment 8542061 [details] [diff] [review]
Allow nested content process to embed apps and add a nested pref check function

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

Please read your patch more carefully before uploading it!

::: dom/html/nsGenericHTMLFrameElement.cpp
@@ +337,5 @@
>      Preferences::AddBoolVarCache(&sMozWidgetsEnabled,
>                                   "dom.enable_widgets");
>    }
>  
> +  return sMozWidgetsEnabled;  static bool sMozWidgetsEnabled = false;

This should be wrong.

@@ +459,5 @@
>  
> +  // Only allow content process to embed an app when nested content process is enabled.
> +  if (XRE_GetProcessType() != GeckoProcessType_Default &&
> +      !(GetBoolAttr(nsGkAtoms::Remote) &&
> +      NestedEnabled())){

nit: !(GetBoolAttr(nsGkAtoms::Remote) && NestedEnabled()) is better for reading.

Comment 10

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Allow nested content process to embed apps and add a nested pref check function

Hi, Kershaw. I've corrected the code and fixed the nit. Could you help me to check this patch again? Thank you.

Comment 11

[Kershaw Chang [:kershaw]]

Comment on attachment 8542470 [details] [diff] [review]
Allow nested content process to embed apps and add a nested pref check function

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

::: dom/html/nsGenericHTMLFrameElement.cpp
@@ +459,5 @@
>  
> +  // Only allow content process to embed an app when nested content process is enabled.
> +  if (XRE_GetProcessType() != GeckoProcessType_Default &&
> +      !(GetBoolAttr(nsGkAtoms::Remote) && NestedEnabled())){
> +    NS_WARNING("Can't embed-apps. Embed-apps is restricted to in-proc apps or when nested content process is enabled, see bug 1097479");

nit: This line is too long. We have to make it two lines.

Comment 12

[Kershaw Chang [:kershaw]]

 > @@ +445,5 @@
> >  
> > +  // Only nested content process is allowed to embed an app.
> > +  if (XRE_GetProcessType() != GeckoProcessType_Default &&
> > +      !GetBoolAttr(nsGkAtoms::Remote) &&
> > +      !Preferences::GetBool("dom.ipc.tabs.nested.enabled", false)) {
> 
> Should avoid this Preferences check by Preference cache. You could find
> examples in the same file like BrowserFramesEnabled() or WidgetsEnabled()
> 
> The test should be
> 
>  IsContentProcess || !(HasRemote && NestedEnabled)

Hi Kan-Ru,

I think the test should be IsContentProcess && !(HasRemote && NestedEnabled), right?
Could you also take a look at Kevin's patch?
Should we create another test case for this bug?

Thanks.

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Kershaw Chang [:kershaw] from comment #12)
>  > @@ +445,5 @@
> > >  
> > > +  // Only nested content process is allowed to embed an app.
> > > +  if (XRE_GetProcessType() != GeckoProcessType_Default &&
> > > +      !GetBoolAttr(nsGkAtoms::Remote) &&
> > > +      !Preferences::GetBool("dom.ipc.tabs.nested.enabled", false)) {
> > 
> > Should avoid this Preferences check by Preference cache. You could find
> > examples in the same file like BrowserFramesEnabled() or WidgetsEnabled()
> > 
> > The test should be
> > 
> >  IsContentProcess || !(HasRemote && NestedEnabled)
> 
> Hi Kan-Ru,
> 
> I think the test should be IsContentProcess && !(HasRemote &&
> NestedEnabled), right?

Ah, Yes.

> Could you also take a look at Kevin's patch?
> Should we create another test case for this bug?

Yes, test case is better.

Comment 14

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: 0001-Bug-1097479-Allow-nested-content-process-to-embed-ap.patch

Hi Kershaw, I add a test. In this test I set value of preference "dom.ipc.tabs.nested.enabled" to ture and append an iframe to check if it can really open a embed remote widget.

There a question, I reuse the file file_browserElement_DisallowEmbedAppInOOP.html in my test, whose filename is not really fit to the test I do. Is it okay that I use this file or I have to create another file?

And please help me to check this test. Thank you.

Comment 15

[Kershaw Chang [:kershaw]]

Comment on attachment 8545071 [details] [diff] [review]
0001-Bug-1097479-Allow-nested-content-process-to-embed-ap.patch

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

We can not reuse file_browserElement_DisallowEmbedAppInOOP.html because the remote attribute is not set to true in this file.

How about test_browserElement_oop_AllowEmbedAppsInNestedOOIframe.html?

::: dom/browser-element/mochitest/browserElement_AllowEmbedAppsInOOPWithPref.js
@@ +6,5 @@
> +//
> +
> +"use strict";
> +
> +SpecialPowers.setBoolPref("dom.ipc.tabs.nested.enabled", false);

This should be true.

@@ +14,5 @@
> +
> +SpecialPowers.setAllAppsLaunchable(true);
> +
> +function runTest() {
> +  var canEmbedApp = (browserElementTestHelpers.getOOPByDefaultPref() & 

nit: trailing space

::: dom/html/nsGenericHTMLFrameElement.cpp
@@ +456,5 @@
>    if (!nsIMozBrowserFrame::GetReallyIsBrowserOrApp()) {
>      return NS_OK;
>    }
>  
> +  // Only allow content process to embed an app when nested content 

nit: trailing space

Comment 16

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: 0001-Bug-1097479-Allow-nested-content-process-to-embed-ap.patch

Hi Kershaw, sory about the nit. I already changed the "dom.ipc.tabs.nested.enabled" and <remote> to the value true, change the file name and a file "file_browserElement_AllowEmbedAppsInOOIframe.html"

Please help me to check this patch. Thank you.

Comment 17

[Kershaw Chang [:kershaw]]

Comment on attachment 8545790 [details] [diff] [review]
0001-Bug-1097479-Allow-nested-content-process-to-embed-ap.patch

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

I'd like Kan-Ru to also have a look.
Just make sure this test case matches what we need.

::: dom/browser-element/mochitest/browserElement_AllowEmbedAppsInNestedOOIframe.js
@@ +14,5 @@
> +SpecialPowers.setAllAppsLaunchable(true);
> +
> +function runTest() {
> +  var canEmbedApp = (browserElementTestHelpers.getOOPByDefaultPref() &
> +                     browserElementTestHelpers._getBoolPref("dom.ipc.tabs.nested.enabled"));

Use SpecialPowers.getBoolPref and there is a redundant().

Comment 18

[Kershaw Chang [:kershaw]]

Hi Kan-Ru,

May I know your feedback about Kevin's patch? If you think it's ok, whom should we ask to review this patch?

Comment 19

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8545790 [details] [diff] [review]
0001-Bug-1097479-Allow-nested-content-process-to-embed-ap.patch

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

Looks good. Fix the comments and I'll review it.

::: dom/browser-element/mochitest/browserElement_AllowEmbedAppsInNestedOOIframe.js
@@ +7,5 @@
> +"use strict";
> +
> +SimpleTest.waitForExplicitFinish();
> +browserElementTestHelpers.setEnabledPref(true);
> +SpecialPowers.setBoolPref("dom.ipc.tabs.nested.enabled", true);

Use SpecialPowers.pushPrefEnv. There are many examples in the same directory.

@@ +14,5 @@
> +SpecialPowers.setAllAppsLaunchable(true);
> +
> +function runTest() {
> +  var canEmbedApp = (browserElementTestHelpers.getOOPByDefaultPref() &
> +                     browserElementTestHelpers._getBoolPref("dom.ipc.tabs.nested.enabled"));

Use && instead of &

Comment 20

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Allow nested content process-to embed apps and add a nested pref check function

Hi Kan-Ru. I took some time to trace the code. Attachment is my modification. Could you help me to review it? Thank you.

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8553564 [details] [diff] [review]
Allow nested content process-to embed apps and add a nested pref check function

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

::: dom/browser-element/mochitest/browserElement_AllowEmbedAppsInNestedOOIframe.js
@@ +9,5 @@
> +SimpleTest.waitForExplicitFinish();
> +browserElementTestHelpers.setEnabledPref(true);
> +SpecialPowers.pushPrefEnv(
> +  {"set": [["dom.ipc.tabs.nested.enabled", true]]},
> +  browserElementTestHelpers.addPermission());

This might not do what you want. browserElementTestHelpers.addPermission() is executed before pushPrefEnv.

@@ +36,5 @@
> +    iframe.src = 'http://example.org/tests/dom/browser-element/mochitest/file_browserElement_AllowEmbedAppsInNestedOOIframe.html';
> +  });
> +}
> +
> +addEventListener('testready', runTest);

Instead, use

 addEventListener('testready', () => {
   SpecialPowers.pushPrefEnv({"set": [["dom.ipc.tabs.nested.enabled", true]]}, runTest);
 });

Comment 22

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Allow nested content process to embed apps and add a nested pref check function

Hi, Kan-Ru. I have modified the code. Could you please help me to check this patch again? thank you.

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8554416 [details] [diff] [review]
Allow nested content process to embed apps and add a nested pref check function

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

Looks good! Remember to push to try before checkin.

Comment 24

[Kevin Chen[:kechen] (UTC + 8)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9491263ef6d

It seems my code can't pass some test cases. I will check my modification again.

Comment 25

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Bug 1097479 - Allow nested content process to embed apps and add a nested pref check function

Hi Kanru, when pushed the codes to the try server, I ran into some error like the following link :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9491263ef6d

After discussing with Kershaw, I changed the test code to run on e10s.
And the result shows that this will fix the error :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=070038f0ed3e

And about the Marionette error, I pushed a additional try without any of my modifications. It causes error, still. So the error might be not related to my code.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b09fcbc23a25

Can you help me to check this modification? Thank you.

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8556916 [details] [diff] [review]
Bug 1097479 - Allow nested content process to embed apps and add a nested pref check function

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

::: dom/browser-element/mochitest/mochitest-oop.ini
@@ +15,5 @@
>  skip-if = toolkit=='gonk' || (toolkit == 'gonk' && !debug)
>  [test_browserElement_oop_Alert.html]
>  [test_browserElement_oop_AlertInFrame.html]
> +[test_browserElement_oop_AllowEmbedAppsInNestedOOIframe.html]
> +skip-if = !e10s

mochitest-oop.ini doesn't run under e10s (under [DEFAULT] section) so this modification effectively disables this test.

Comment 27

[Kan-Ru Chen [:kanru] (UTC+9)]

The error is this line

19:43:20     INFO -  JavaScript error: jar:file:///builds/slave/test/build/application/firefox/omni.ja!/components/BrowserElementParent.js, line 163: NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]

Comment 28

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Allow nested content process to embed apps and add a nested pref check function

Hi KanRu, can you help me to review this patch?
This patch is quite the same with attachment 8554416 [details] [diff] [review], but we ran into a work queue problem then(Bug 1131444), but it is fixed now.
In this patch I modify the check condition to allow content process to embed an app.
And also, a mochitest for this modification. However, B2G is not totally ready for supporting nested-oop, so the test case will skip it currently.

Comment 29

[Kevin Chen[:kechen] (UTC + 8)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53665158069e
There are some errors in try run. I am checking them now, but seems not related to my modification.

Comment 30

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8599132 [details] [diff] [review]
Allow nested content process to embed apps and add a nested pref check function

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

Does this test run in-process? If so, please also add a _inproc_ version.

::: dom/browser-element/mochitest/browserElement_AllowEmbedAppsInNestedOOIframe.js
@@ +13,5 @@
> +SpecialPowers.setAllAppsLaunchable(true);
> +
> +function runTest() {
> +  var canEmbedApp = browserElementTestHelpers.getOOPByDefaultPref() &&
> +                    SpecialPowers.getBoolPref("dom.ipc.tabs.nested.enabled");

Looks like we could also check canEmbedApp == false when this test run in-process.

Comment 31

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Bug 1097479 - Allow nested content process to embed apps and add a nested pref check function.

Carry r+ from comment 30 .
Change the test condition 
is(e.detail.message == 'app', canEmbedApp, e.detail.message); to 
is(e.detail.message == 'app', true, e.detail.message);
because canEmbedApp should always be true in oop situation.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69cff2238ff9

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/69cff2238ff9

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/69cff2238ff9