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

Hi Kan-Ru, Could you take a look at this patch? Thanks.

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)

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

Sure. Feel free to do this.

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

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.

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

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

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.

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

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.

> @@ +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.

(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.

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

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 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().

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 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 &

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 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); });

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

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.

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.

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 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.

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]

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.

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 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.

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.