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

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kanru, Assigned: kechen)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla40
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 10 obsolete attachments)

8.18 KB, patch
kechen
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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
Assignee: nobody → kechang
Created attachment 8538353 [details] [diff] [review]
Allow nested content process to embed apps

Hi Kan-Ru,

Could you take a look at this patch?

Thanks.
Attachment #8538353 - Flags: feedback?(kchen)
(Reporter)

Comment 2

4 years ago
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)
Attachment #8538353 - Flags: feedback?(kchen)
(Assignee)

Comment 3

4 years ago
Hi Kershaw,I would like to practice fixing some bugs. Is it okay that I take this bug? Thanks
Flags: needinfo?(kechang)
Sure. Feel free to do this.
Flags: needinfo?(kechang)
(Assignee)

Updated

4 years ago
Assignee: kechang → kechen
(Assignee)

Comment 5

4 years ago
Created attachment 8541973 [details] [diff] [review]
Allow nested content process to embed apps

Hi Kershaw, could you help me to check this patch? thank you.
Attachment #8541973 - Flags: feedback?(kechang)
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.
Attachment #8541973 - Flags: feedback?(kechang) → feedback-
BTW, please also make the old patches obsolete when you upload a new one.
Thanks.
(Assignee)

Comment 8

4 years ago
Created attachment 8542061 [details] [diff] [review]
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.
Attachment #8542061 - Flags: feedback?(kechang)
(Assignee)

Updated

4 years ago
Attachment #8541973 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8541973 - Attachment is obsolete: false
(Assignee)

Updated

4 years ago
Attachment #8541973 - Attachment is obsolete: true
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.
Attachment #8542061 - Flags: feedback?(kechang) → feedback?
Attachment #8542061 - Flags: feedback?
(Assignee)

Updated

4 years ago
Attachment #8538353 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8542061 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Created attachment 8542470 [details] [diff] [review]
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.
Attachment #8542470 - Flags: feedback?(kechang)
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.
Attachment #8542470 - Flags: feedback?(kechang) → feedback+
 > @@ +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.
Attachment #8542470 - Flags: feedback+ → feedback?(kchen)
(Reporter)

Comment 13

4 years ago
(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.
(Reporter)

Updated

4 years ago
Attachment #8542470 - Flags: feedback?(kchen) → feedback+
(Assignee)

Comment 14

4 years ago
Created attachment 8545071 [details] [diff] [review]
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.
Attachment #8545071 - Flags: feedback?(kechang)
(Assignee)

Updated

4 years ago
Attachment #8542470 - Attachment is obsolete: true
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
Attachment #8545071 - Flags: feedback?(kechang)
(Assignee)

Comment 16

4 years ago
Created attachment 8545790 [details] [diff] [review]
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.
Attachment #8545790 - Flags: feedback?(kechang)
(Assignee)

Updated

4 years ago
Attachment #8545071 - Attachment is obsolete: true
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().
Attachment #8545790 - Flags: feedback?(kechang)
Attachment #8545790 - Flags: feedback?(kchen)
Attachment #8545790 - Flags: feedback+
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?
Flags: needinfo?(kchen)
(Reporter)

Comment 19

4 years ago
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 &
Attachment #8545790 - Flags: feedback?(kchen) → feedback+
(Reporter)

Updated

4 years ago
Flags: needinfo?(kchen)
(Assignee)

Comment 20

4 years ago
Created attachment 8553564 [details] [diff] [review]
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.
Attachment #8545790 - Attachment is obsolete: true
Attachment #8553564 - Flags: review?(kchen)
(Reporter)

Comment 21

4 years ago
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);
 });
Attachment #8553564 - Flags: review?(kchen)
(Assignee)

Comment 22

4 years ago
Created attachment 8554416 [details] [diff] [review]
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.
Attachment #8553564 - Attachment is obsolete: true
Attachment #8554416 - Flags: review?(kchen)
(Reporter)

Comment 23

4 years ago
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.
Attachment #8554416 - Flags: review?(kchen) → review+
(Assignee)

Comment 24

4 years ago
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.
(Assignee)

Comment 25

4 years ago
Created attachment 8556916 [details] [diff] [review]
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.
Attachment #8554416 - Attachment is obsolete: true
Flags: needinfo?(kchen)
(Reporter)

Comment 26

4 years ago
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.
Attachment #8556916 - Flags: feedback-
(Reporter)

Comment 27

4 years ago
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]
Flags: needinfo?(kchen)
(Assignee)

Updated

4 years ago
Depends on: 1138793
No longer depends on: 1059662
(Assignee)

Comment 28

4 years ago
Created attachment 8599132 [details] [diff] [review]
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.
Attachment #8556916 - Attachment is obsolete: true
Attachment #8599132 - Flags: review?(kchen)
(Assignee)

Comment 29

4 years ago
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.
(Reporter)

Comment 30

4 years ago
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.
Attachment #8599132 - Flags: review?(kchen) → review+
(Assignee)

Comment 31

4 years ago
Created attachment 8599245 [details] [diff] [review]
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.
Attachment #8599132 - Attachment is obsolete: true
Attachment #8599245 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/69cff2238ff9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.