A new 'embed-widgets' permission exposing to privileged apps for solving widget case.

VERIFIED FIXED in 2.1 S3 (29aug)

Status

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: GaryChen, Assigned: junior)

Tracking

({dev-doc-needed})

unspecified
2.1 S3 (29aug)
x86
macOS
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap -

Firefox Tracking Flags

(feature-b2g:2.1)

Details

(Whiteboard: [FT:Stream3][2.1-feature-qa+])

Attachments

(2 attachments, 17 obsolete attachments)

28.06 KB, patch
junior
: review+
junior
: superreview+
Details | Diff | Splinter Review
29.21 KB, patch
junior
: review+
Details | Diff | Splinter Review
Let 3rd-party APP has ability to embed "OPEN WEB APPs” in mozAPP frames.
Such as homescreem, lockscreen....etc.
As specified on the dev-webapi mailing list, one of the only thing I can think of that will be an issue is the code at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp#228

One issue with just exposing it as if is that apps basically also needs the webapps-manage permission in order to iterate over app manifests to retrieve the list of widgets and serve the right widget based on the locale.

An other issue would be that embedding apps may get some informations from the iframe url by reading the src, and some apps may leak some private informations.

Comment 2

5 years ago
Jonas's idea of a new "embed-widgets" permission[1] gets most people's feedback+ on dev-webapi mailing list, so I'd like to modify the title of this bug to reflect the discussion has moved to the new "embed-widgets" permission instead of "embed-apps".

Gary will summarize our discussion there and update on this bug, as well as create a wiki page and write down more details, like: widget use cases, and the usage of combining browser and homescreen-webapps-manager permissions ... etc.

[1] https://groups.google.com/d/msg/mozilla.dev.webapi/mSkywh1P7L8/Z9_8qQN9pkwJ
Assignee: nobody → gchen
Summary: expose 'embed-apps' to privilege. → A new 'embed-widgets' permission exposing to privileged apps for solving widget case.
Assignee

Comment 3

5 years ago
I'll try to let Gecko support "embed-widgets" permission and relative function by following steps:
Part1: Add a permission "embed-widgets" and an HTMLIFrameElement attribute "mozWidget"
Part2: Implementation of checking if a window is able to embed a specific widget.
Part3: Enable to embed a widget if preconditions hold
Part4: Only limited browser API are available to a widget
Assignee

Updated

5 years ago
Assignee: gchen → juhsu
feature-b2g: --- → 2.1

Comment 4

5 years ago
Is there any reason for not granting this to all apps by default? It seems that the 'Safer' Browser API is no danger than normal iframe.
Blocks: Stream3_2.1
Whiteboard: [FT:Stream3]
Assignee

Comment 5

5 years ago
WIP patch
Part 1.1: Load a widget as an app if the |src| is in the |widgetPages|
* * *
1. Add permission |embed-widgets| and Element attribute |mozwidget|
2. Add |hasWidgetPage| in /mozIApplication.idl
3. Check permission |embed-widgets| and the |src| is in the |widgetPages| when |GetAppManifest|
* * *
Todo List:
1. Test Case
2. |entry_points| in communications app (dialer/contacts...) is not handled.
3. Some behaviour needs to be clairfied.

Hello Fabrice, would you be able to feedback the patch?
Thanks.
Attachment #8450790 - Flags: feedback?(fabrice)
Assignee

Comment 6

5 years ago
There's an ambiguous behaviour need to be clairfied.
In the WIP patch, if the |src| is not listed in the |widgetPages| list, the iframe will be a general iframe, not a widget/app.
Is the behaviour reasonable? Or it should be a blank page.
Flags: needinfo?(jonas)
Assignee

Comment 7

5 years ago
Follow by comment 5
Fixed some symbol error
Attachment #8450790 - Attachment is obsolete: true
Attachment #8450790 - Flags: feedback?(fabrice)
Attachment #8450898 - Flags: feedback?(fabrice)
Comment on attachment 8450898 [details] [diff] [review]
Part 1.1: Load a widget as an app if the |src| is in the |widgetPages|

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

::: content/html/content/src/nsGenericHTMLFrameElement.cpp
@@ +361,5 @@
> +    }
> +
> +    manifestURL.Assign(appManifestURL);
> +    isApp = true;
> +  } while(0);

do / while(0) is a bit ugly. Why no do:
{
...
if (!appManifestURL.IsEmpty()) {
  manifestURL.Assign(appManifestURL);
  isApp = true;
}

@@ +385,5 @@
> +    isWidget = true;
> +  } while(0);
> +
> +  if( (!isApp && !isWidget)  /* No valid case */
> +      || (isApp && isWidget) /* Conflicted */) {

I would log something here.

@@ +404,5 @@
> +  nsAutoString src;
> +  if (isWidget) {
> +    GetAttr(kNameSpaceID_None, nsGkAtoms::src, src);
> +
> +    app->HasWidgetPage(src, &hasWidgetPage);

check that HasWidget() succeeds.

::: dom/apps/src/AppsUtils.jsm
@@ +58,5 @@
>    },
>  
> +  hasWidgetPage: function(aPage) {
> +    let originURI = Services.io.newURI(this.origin, null, null);
> +    for(let id in this.widgetPages) {

nit: for (...)

@@ +688,5 @@
> +  get widgetPages() {
> +    if (this._manifest.widgetPages) {
> +      return this._manifest.widgetPages;
> +    }
> +    return [];

hm... Ithink should we support localized widgets. We support that for launch_path, and widgets are similar.

::: dom/apps/src/Webapps.jsm
@@ +332,5 @@
>          }
>          let app = this.webapps[aResult.id];
>          app.csp = aResult.manifest.csp || "";
>          app.role = aResult.manifest.role || "";
> +        app.widgetPages = aResult.widgetPages || [];

Here and in the other places you create the array: I would rather store the full uris of all the recognized widgets. Not sure how we want to support localization either... That needs to be discussed.

::: dom/interfaces/apps/mozIApplication.idl
@@ +18,5 @@
>    boolean hasPermission(in string permission);
>  
> +  /* Return true if this app can be a widget and
> +     its |widgetPages| contains |page|  */
> +  boolean hasWidgetPage(in DOMString page);

nit: s/page/pageURL
Attachment #8450898 - Flags: feedback?(fabrice)
All,

I had tried to migrate the stingray homescreen to adapt this WIP patch of widget API. The following branch is my current work on it:

https://github.com/huchengtw-moz/gaia/tree/stingray-widget-api
Since the scope of this WIP patch, I had done the following tests:
1. create a widget iframe with mozwidget attribute
    works as expected
2. create a widget iframe without mozwidget attribute
    the iframe becomes normal iframe, as expected.
3. remove the embed-widgets permission from homescreen manifest
    the iframe becomes normal iframe, as expected.
4. request embed-apps and embed-widgets permission both in homescreen manifest
    works as expected
5. add both mozwidget and mozapp attributes to iframe
    the iframe becomes normal iframe, as expected.
6. the src of widget iframe is not in widgetPages
    the iframe becomes normal iframe, as expected.
BTW, the expected behavior of item 6 of comment 10 is based on current implementation.
I had made a case to test widgetPages, in https://github.com/huchengtw-moz/gaia/tree/stingray-widget-api-iframe-navigation

The behavior looks wrong:
1. create a widget whose src is not in widgetPages and navigate to page who is in widgetPages
    Result: all pages didn't get permissions.
2. create a widget whose src is in widgetPages and navigate to page who is not in widgetPages
    Result: all pages got permissions.
Assignee

Comment 13

5 years ago
(In reply to Fabrice Desré [:fabrice] from comment #8)
> Comment on attachment 8450898 [details] [diff] [review]
> Part 1.1: Load a widget as an app if the |src| is in the |widgetPages|
>
> Review of attachment 8450898 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: content/html/content/src/nsGenericHTMLFrameElement.cpp
> @@ +361,5 @@
> > +    }
> > +
> > +    manifestURL.Assign(appManifestURL);
> > +    isApp = true;
> > +  } while(0);
>
> do / while(0) is a bit ugly. Why no do:
> {
> ...
> if (!appManifestURL.IsEmpty()) {
>   manifestURL.Assign(appManifestURL);
>   isApp = true;
> }
>
The reason of using |do / while(0)| is multiple |breaks|.
i.e. |isApp=true| if multiple conditions hold.
AFAIK, alternative choices are
1. nested if blocks: hard to trace
2. goto: infrequently used in mozilla
3. check all the conditions first; finally check if all conditions hold.
   ==> performance issue

I am wondering which one is prefered.

> ::: dom/apps/src/Webapps.jsm
> @@ +332,5 @@
> >          }
> >          let app = this.webapps[aResult.id];
> >          app.csp = aResult.manifest.csp || "";
> >          app.role = aResult.manifest.role || "";
> > +        app.widgetPages = aResult.widgetPages || [];
>
> Here and in the other places you create the array: I would rather store the
> full uris of all the recognized widgets. Not sure how we want to support
> localization either... That needs to be discussed.
>

1. 
The main reason of the memory-consuming mechanism is that |getManifestFor| returns a Promise, which is un-synchronizable.
http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsService.js#43
Therefore, it's necessary to store all the |widgetPages| as load/update/clone. 
(Otherwise we need to make Promise sync in cpp)

2. 
We have discuss a little bit about L10n, which is in the following link.
https://wiki.mozilla.org/WebAPI/WidgetAPI#extend_manifest.webapp
L10n has not been tested in this patch.
Flags: needinfo?(fabrice)
(In reply to Junior Hsu [:juniorhsu] from comment #13)
> (In reply to Fabrice Desré [:fabrice] from comment #8)
> > Comment on attachment 8450898 [details] [diff] [review]
> > Part 1.1: Load a widget as an app if the |src| is in the |widgetPages|
> >
> > Review of attachment 8450898 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > ::: content/html/content/src/nsGenericHTMLFrameElement.cpp
> > @@ +361,5 @@
> > > +    }
> > > +
> > > +    manifestURL.Assign(appManifestURL);
> > > +    isApp = true;
> > > +  } while(0);
> >
> > do / while(0) is a bit ugly. Why no do:
> > {
> > ...
> > if (!appManifestURL.IsEmpty()) {
> >   manifestURL.Assign(appManifestURL);
> >   isApp = true;
> > }
> >
> The reason of using |do / while(0)| is multiple |breaks|.
> i.e. |isApp=true| if multiple conditions hold.
> AFAIK, alternative choices are
> 1. nested if blocks: hard to trace
> 2. goto: infrequently used in mozilla
> 3. check all the conditions first; finally check if all conditions hold.
>    ==> performance issue

Or 4. don't inline these blocks, but turn them into functions that will return a boolean assigned to isApp and isWidget.

> 1. 
> The main reason of the memory-consuming mechanism is that |getManifestFor|
> returns a Promise, which is un-synchronizable.
> http://dxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsService.js#43
> Therefore, it's necessary to store all the |widgetPages| as
> load/update/clone. 
> (Otherwise we need to make Promise sync in cpp)

I don't follow you here. You have the manifest already (it's aResult.manifest). And you could just resolve the relative urls against the manifestURL.

> 2. 
> We have discuss a little bit about L10n, which is in the following link.
> https://wiki.mozilla.org/WebAPI/WidgetAPI#extend_manifest.webapp
> L10n has not been tested in this patch.

I'd like that to be figured out before we land this patch.
Flags: needinfo?(fabrice)
Assignee

Comment 15

5 years ago
WIP patch

Modifications:
1. Add test case
2. Update according to feedback comment

Todo list:
1. |entry_points| in communications app (dialer/contacts...) is not handled
2. Some behaviour should be determined (L10n, src not in widgetPages)
3. Changes in Webapps.jsm should be investigated
Attachment #8450898 - Attachment is obsolete: true
Attachment #8454349 - Flags: feedback?(fabrice)
Comment on attachment 8454349 [details] [diff] [review]
Part 1: Load a widget as an app if the |src| is in the |widgetPages|

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

much better!

::: content/html/content/src/nsGenericHTMLFrameElement.cpp
@@ +325,5 @@
>  
> +/* @param AppType: nsGkAtoms::mozapp or nsGkAtoms::mozwidget
> + * */
> +void nsGenericHTMLFrameElement::GetManifestURLByType(nsIAtom *aAppType,
> +                                                     bool *aIsOfAppType,

the parameter names are a bit confusing I think. aOut should be aManifestURL

@@ +340,5 @@
> +  nsCOMPtr<nsIPermissionManager> permMgr =
> +    services::GetPermissionManager();
> +  NS_ENSURE_TRUE_VOID(permMgr);
> +  nsIPrincipal *principal = NodePrincipal();
> +  const char* aPermissionType = (aAppType==nsGkAtoms::mozapp) ?

nit: spaces around ==

::: dom/apps/src/AppsUtils.jsm
@@ +60,5 @@
> +  hasWidgetPage: function(aPageURL) {
> +    let originURI = Services.io.newURI(this.origin, null, null);
> +    for (let id in this.widgetPages) {
> +      let widgetPage = this.widgetPages[id];
> +      if(widgetPage && originURI.resolve(widgetPage) === aPageURL) {

That is is the critical path of nsGenericHTMLFrameElement::GetAppManifestURL() and you could save the uri resolution if you stored the resolved path (they don't change) during execution.

::: dom/apps/tests/test_widget.html
@@ +79,5 @@
> +    // Preferences
> +    function() {
> +      SpecialPowers.pushPrefEnv({"set": [["dom.datastore.enabled", true],
> +                                         ["dom.testing.ignore_ipc_principal", true],
> +                                         ["dom.testing.datastore_enabled_for_hosted_apps", true]]}, runTest);

why do we need these datastore prefs?
Attachment #8454349 - Flags: feedback?(fabrice) → feedback+
Assignee

Comment 17

5 years ago
WIP patch

Update according to feedback comment 14, mostly in WebApps.jsm

Todo list:
Some behaviour should be determined (L10n, src not in widgetPages)
Attachment #8454349 - Attachment is obsolete: true
Attachment #8456080 - Flags: feedback?(fabrice)
Hi Fabrice, I'm thinking if we can separate l10n support into a follow-up bug? Gaia developers and partners want a platform for their widget development as early as possible.
Flags: needinfo?(fabrice)
(In reply to Shih-Chiang Chien [:schien] (UTC+8) from comment #18)
> Hi Fabrice, I'm thinking if we can separate l10n support into a follow-up
> bug? Gaia developers and partners want a platform for their widget
> development as early as possible.

This should not be hard to decide right? We already have a l10n system in manifests. You can reuse that, or if you don't want tell me why.
Flags: needinfo?(fabrice)
Comment on attachment 8456080 [details] [diff] [review]
Part 1: Load a widget as an app if the |src| is in the |widgetPages|

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

::: content/html/content/src/nsGenericHTMLFrameElement.cpp
@@ +341,5 @@
> +    services::GetPermissionManager();
> +  NS_ENSURE_TRUE_VOID(permMgr);
> +  nsIPrincipal *principal = NodePrincipal();
> +  const char* aPermissionType = (aAppType == nsGkAtoms::mozapp) ?
> +                                "embed-apps" : "embed-widgets";

nit: check the style guide or other uses of the ternary operator in this file. My preference is usually to align '?' and ':' like:
const char* aPermissionType = (aAppType == nsGkAtoms::mozapp) ? "embed-apps"
                                                              : "embed-widgets";

::: dom/apps/src/Webapps.jsm
@@ +308,5 @@
>      }
>      return res.length > 0 ? res : null;
>    },
>  
> +  _saveFullPathOfWidgetPages: function(aOrigin, aManifest, aDestApp) {

Please resolve the uris against the manifest url instead of the origin. Also, rename the method to _saveWidgetsFullPath()

@@ +309,5 @@
>      return res.length > 0 ? res : null;
>    },
>  
> +  _saveFullPathOfWidgetPages: function(aOrigin, aManifest, aDestApp) {
> +    var originURI = Services.io.newURI(aOrigin, null, null);

s/var/let

@@ +340,5 @@
>          }
>          let app = this.webapps[aResult.id];
>          app.csp = aResult.manifest.csp || "";
>          app.role = aResult.manifest.role || "";
> +        this._saveFullPathOfWidgetPages(app.origin, aResult.manifest, app);

the manifest here is not wrapped in a ManifestHelper(), so you won't get locale processing.

@@ +968,5 @@
>  
>          app.name = manifest.name;
>          app.csp = manifest.csp || "";
>          app.role = localeManifest.role;
> +        this._saveFullPathOfWidgetPages(app.origin, manifest, app);

you need to use localeManifest here.

@@ +2385,5 @@
>      appObject.basePath = OS.Path.dirname(this.appsFile);
>      appObject.name = aManifest.name;
>      appObject.csp = aLocaleManifest.csp || "";
>      appObject.role = aLocaleManifest.role;
> +    appObject.widgetPages = aManifest.widgetPages || [];

s/aManifest/aLocaleManifest. But is that even correct to not use _saveFullPathOfWidgetPages here ?

@@ +2421,5 @@
>      app.name = aManifest.name;
>  
>      app.csp = aManifest.csp || "";
>  
> +    app.widgetPages = aManifest.widgetPages || [];

same here
Attachment #8456080 - Flags: feedback?(fabrice) → feedback+
(In reply to Fabrice Desré [:fabrice] from comment #19)
> This should not be hard to decide right? We already have a l10n system in
> manifests. You can reuse that, or if you don't want tell me why.

Hi Fabrice,

The benefits for spliting some efforts into follow up bugs are
  1. To land the main core of widget support as early as possible but be disabled by preference.
     Then partner can help us to do the test based on their use cases.
  2. We can find extra resources to fix these follow up bugs in pararell based on core patches here.
  3. Junior can more focus on implemeting main core of widget support.

This is the suggestion only and respect for reviewer's decision.

Thanks.
(In reply to Marco Chen [:mchen] from comment #21)

> The benefits for spliting some efforts into follow up bugs are
>   1. To land the main core of widget support as early as possible but be
> disabled by preference.
>      Then partner can help us to do the test based on their use cases.
>   2. We can find extra resources to fix these follow up bugs in pararell
> based on core patches here.
>   3. Junior can more focus on implemeting main core of widget support.

Anyway all is fine since the latest patch does the right thing to get the localized widgets from manifests.
Assignee

Comment 23

5 years ago
1. Add permission |embed-widgets| and Element attribute |mozwidget|
2. Add |hasWidgetPage| in mozIApplication.idl
3. Check permission |embed-widgets| and the |src| is in the |widgetPages| when |GetAppManifest|
4. Add test case
5. Should enable preference |dom.enable_widgets|
 
Note.
1. In this patch, if the src is not in the |widgetPages| specified in manifest, the widget is without priviledged/certificated webapi.
2. To enable the widget functionality, preference |dom.enable_widgets| should be enabled.
Attachment #8456080 - Attachment is obsolete: true
Attachment #8458427 - Flags: superreview?(jonas)
Attachment #8458427 - Flags: review?(fabrice)
Assignee

Comment 24

5 years ago
(In reply to Fabrice Desré [:fabrice] from comment #20)
> @@ +2385,5 @@
> >      appObject.basePath = OS.Path.dirname(this.appsFile);
> >      appObject.name = aManifest.name;
> >      appObject.csp = aLocaleManifest.csp || "";
> >      appObject.role = aLocaleManifest.role;
> > +    appObject.widgetPages = aManifest.widgetPages || [];
> 
> s/aManifest/aLocaleManifest. But is that even correct to not use
> _saveFullPathOfWidgetPages here ?
> 

The code snippet is in _cloneApp, which is only called by ConfirmInstall.
Originally, it makes more sense to use saveFullPathOfWidgetPages in |ConfirmInstall| IMO.
Now I prefer to put saveFullPathOfWidgetPages in _cloneApp since it's error-free to reuse it, just matching your suggestion.


To explain more on Comment 23, the behaviour of Note 1 is not determined yet, thus causing me to implement an intuitive one first.
Comment on attachment 8458427 [details] [diff] [review]
Part 1: Load a widget as an app if the |src| is in the |widgetPages|

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

Very close, but I want to take a last look before r+.
Can you also push your patch to the try server? thanks!

::: content/html/content/src/nsGenericHTMLFrameElement.cpp
@@ +325,5 @@
>    return NS_OK;
>  }
>  
> +/* @param AppType: nsGkAtoms::mozapp or nsGkAtoms::mozwidget
> + * */

nit: strange comment formatting here.

@@ +333,5 @@
> +{
> +  *aIsOfAppType = false;
> +  aManifestURL.Truncate();
> +
> +  if(aAppType != nsGkAtoms::mozapp && aAppType != nsGkAtoms::mozwidget) {

nit: here and in many other cases: |if (...)|

@@ +381,4 @@
>  
> +  GetManifestURLByType(nsGkAtoms::mozapp, &isApp, appManifestURL);
> +
> +  //Need Pref for Widget Enable

nit: // Check if the widget preference is enabled.

@@ +381,5 @@
>  
> +  GetManifestURLByType(nsGkAtoms::mozapp, &isApp, appManifestURL);
> +
> +  //Need Pref for Widget Enable
> +  bool isWidgetEnable = false;

s/isWidgetEnable/isWidgetEnabled

@@ +390,5 @@
> +
> +    if (branch) {
> +      branch->GetBoolPref("dom.enable_widgets", &isWidgetEnable);
> +    }
> +  }

Instead of using the preference service like that, please use Preferences::GetBool()

@@ +397,5 @@
> +    GetManifestURLByType(nsGkAtoms::mozwidget, &isWidget, widgetManifestURL);
> +  }
> +
> +  if(!isApp && !isWidget) {
> +    /* No valid case to get manifest*/

nit: // No valid case to get manifest.

@@ +429,5 @@
> +  if (isWidget) {
> +    GetAttr(kNameSpaceID_None, nsGkAtoms::src, src);
> +    nsresult rv = app->HasWidgetPage(src, &hasWidgetPage);
> +
> +    if(NS_SUCCEEDED(rv) && !hasWidgetPage) {

nit: if (...)
Attachment #8458427 - Flags: review?(fabrice) → feedback+
Assignee

Comment 26

5 years ago
Update according to feedback comment 25

Try Result: https://tbpl.mozilla.org/?tree=Try&rev=e32d60ee9bdc
Attachment #8458427 - Attachment is obsolete: true
Attachment #8458427 - Flags: superreview?(jonas)
Attachment #8460156 - Flags: superreview?(jonas)
Attachment #8460156 - Flags: review?(fabrice)
Comment on attachment 8460156 [details] [diff] [review]
Part 1: Load a widget as an app if the |src| is in the |widgetPages|

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

Looks good to me. Let's wait for Jonas before landing. Also, did you file followups for the navigation issues?

::: content/html/content/src/nsGenericHTMLFrameElement.cpp
@@ +338,5 @@
> +  }
> +
> +  // Check permission.
> +  nsCOMPtr<nsIPermissionManager> permMgr =
> +    services::GetPermissionManager();

nit: that should fit on a single line.

::: dom/interfaces/apps/mozIApplication.idl
@@ +17,5 @@
>    /* Return true if this app has |permission|. */
>    boolean hasPermission(in string permission);
>  
> +  /* Return true if this app can be a widget and
> +     its |widgetPages| contains |page|  */

Nit: Multi line comments are formatted this way:
/**
 * Line 1
 * Line 2
 */
Attachment #8460156 - Flags: review?(fabrice) → review+
Assignee

Updated

5 years ago
Blocks: 1043110
Assignee

Comment 28

5 years ago
(In reply to Fabrice Desré [:fabrice] from comment #27)
> Comment on attachment 8460156 [details] [diff] [review]
> Part 1: Load a widget as an app if the |src| is in the |widgetPages|
> 
> Review of attachment 8460156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. Let's wait for Jonas before landing. Also, did you file
> followups for the navigation issues?
> 
I just create the bug 1043110.

And I will implement the limited BrowserAPI feature first:
only limited browser APIs are available to a widget.
https://wiki.mozilla.org/WebAPI/WidgetAPI#Limited_Browser_API

> ::: content/html/content/src/nsGenericHTMLFrameElement.cpp
> @@ +338,5 @@
> > +  }
> > +
> > +  // Check permission.
> > +  nsCOMPtr<nsIPermissionManager> permMgr =
> > +    services::GetPermissionManager();
> 
> nit: that should fit on a single line.
> 
> ::: dom/interfaces/apps/mozIApplication.idl
> @@ +17,5 @@
> >    /* Return true if this app has |permission|. */
> >    boolean hasPermission(in string permission);
> >  
> > +  /* Return true if this app can be a widget and
> > +     its |widgetPages| contains |page|  */
> 
> Nit: Multi line comments are formatted this way:
> /**
>  * Line 1
>  * Line 2
>  */
Hi Jonas,

According to one week passed, set needinfo flag to you.
And please help to give superreivew on mozWidget. Very thanks.
Flags: needinfo?(jonas)
Assignee

Comment 30

5 years ago
Sorry for an additional patch modification.

The modification is for:
1. consistency signature style of GetManifestURLByType and GetAppManifestURL
2. make GetManifestURLByType more senses to me: move all checking detail to it, leave GetAppManifestURL for high level checks

All these changes are in the two mentioned functions in nsGenericHTMLFrameElement.

Due to a little logic modification,I set review? again.

I also rebase and update according to comment 27
Attachment #8460156 - Attachment is obsolete: true
Attachment #8460156 - Flags: superreview?(jonas)
Attachment #8464552 - Flags: superreview?(jonas)
Attachment #8464552 - Flags: review?(fabrice)
Assignee

Comment 31

5 years ago
Try result 
https://tbpl.mozilla.org/?tree=Try&rev=ba3598bb9937

I enable preference dom.enable_widgets in this try test.
Assignee

Comment 32

5 years ago
a WIP patch

I have hidden the methods of Browser API to a widget in the WIP patch, but 
hiding mozbrowser events is not done yet.
Assignee

Comment 33

5 years ago
a WIP patch

Hide security-sensitive mozbrowser events except:
* mozbrowserusernameandpasswordrequired
* mozbrowseropenwindow
* mozbrowserasyncscroll
Attachment #8464592 - Attachment is obsolete: true
Comment on attachment 8464552 [details] [diff] [review]
Part 1: Load a widget as an app if the |src| is in the |widgetPages|

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

Sorry for not catching that earlier, but I'd like to take a last look at the Webapps.jsm changes.

::: content/html/content/src/nsGenericHTMLFrameElement.cpp
@@ +353,5 @@
> +  nsAutoString manifestURL;
> +  GetAttr(kNameSpaceID_None, aAppType, manifestURL);
> +  if (manifestURL.IsEmpty()) {
> +    return;
> +  }

you could move this check earlier in the function (before the permission check).

::: dom/apps/src/Webapps.jsm
@@ +322,5 @@
>  
> +  _saveWidgetsFullPath: function(aManifest, aDestApp) {
> +    let manifestURI = Services.io.newURI(aDestApp.manifestURL, null, null);
> +    if (aManifest.widgetPages) {
> +      aDestApp.widgetPages = aManifest.widgetPages.map(manifestURI.resolve);

that's not correct. You need to build a ManifestHelper object and use its resolveFromOrigin (soon to be resolveURL once bug 1042881 lands) method.

@@ +353,5 @@
>          let app = this.webapps[aResult.id];
>          app.csp = aResult.manifest.csp || "";
>          app.role = aResult.manifest.role || "";
> +
> +        let localeManifest = new ManifestHelper(aResult.manifest, app.origin);

Make sure you update that after bug 1042881 (you need to add app.manifestURL as a 3rd parameter).

@@ +2505,5 @@
>      app.name = aManifest.name;
>  
>      app.csp = aManifest.csp || "";
>  
> +    let aLocaleManifest = new ManifestHelper(aManifest, app.origin);

here too.
Attachment #8464552 - Flags: review?(fabrice)
Assignee

Comment 35

5 years ago
Rebase and update according to comment 34

Try result: https://tbpl.mozilla.org/?tree=Try&rev=f14d29e69c54
Attachment #8464552 - Attachment is obsolete: true
Attachment #8464552 - Flags: superreview?(jonas)
Attachment #8466939 - Flags: superreview?(jonas)
Attachment #8466939 - Flags: review?(fabrice)
QA Whiteboard: [2.1-feature-qa+]
Comment on attachment 8466939 [details] [diff] [review]
Part 1: Load a widget as an app if the |src| is in the |widgetPages|

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

No need to review again, but fix the last nit before landing, thanks!

::: dom/apps/src/Webapps.jsm
@@ +325,5 @@
>      return res.length > 0 ? res : null;
>    },
>  
> +  _saveWidgetsFullPath: function(aManifest, aDestApp) {
> +    let manifestURI = Services.io.newURI(aDestApp.manifestURL, null, null);

Remove this as it's unused.
Attachment #8466939 - Flags: review?(fabrice) → review+
Assignee

Comment 37

5 years ago
Rebase and fix nit according to comment 36, carry r+
Attachment #8466939 - Attachment is obsolete: true
Attachment #8466939 - Flags: superreview?(jonas)
Attachment #8468177 - Flags: superreview?(jonas)
Attachment #8468177 - Flags: review+
QA Whiteboard: [2.1-feature-qa+] → [2.1-feature-qa?]
Assignee

Comment 38

5 years ago
1. Add |ownerIsWidget| in nsIFrameLoader.idl
2. Add |GetReallyIsWidget| in nsIMozBrowserFrame.idl
3. Hide the methods of browser API of a widget
4. Hide security-sensitive mozbrowser events of a widget
* * *
Todo List:
1. Test Case
2. Modify the test of Part1 since |mozbrowsershowmodalprompt| is not available after this patch
Attachment #8465299 - Attachment is obsolete: true
Attachment #8469182 - Flags: feedback?(fabrice)
Assignee

Comment 39

5 years ago
Modify a build time error
Attachment #8469182 - Attachment is obsolete: true
Attachment #8469182 - Flags: feedback?(fabrice)
Attachment #8469938 - Flags: feedback?(fabrice)
QA Whiteboard: [2.1-feature-qa?]
Whiteboard: [FT:Stream3] → [FT:Stream3][2.1-feature-qa+]
Flags: in-moztrap?(fyen)
QA Contact: fyen
Comment on attachment 8469938 [details] [diff] [review]
Part 2: Only limited browser API are available to a widget

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

Redirecting to Kanru that owns the mozbrowser api.
Attachment #8469938 - Flags: feedback?(fabrice) → feedback?(kchen)
Assignee

Updated

5 years ago
Blocks: 1052328
Assignee

Comment 41

5 years ago
Modify test:
1. Use message manager instead of alert owing to no mozbrowsershowmodalprompt events.
2. Add testing limited browser api
Attachment #8469938 - Attachment is obsolete: true
Attachment #8469938 - Flags: feedback?(kchen)
Attachment #8471468 - Flags: review?(kchen)
Comment on attachment 8471468 [details] [diff] [review]
Part 2(v1): Only limited browser API are available to a widget

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

::: content/base/public/nsIFrameLoader.idl
@@ +286,5 @@
> +
> +  /**
> +   * Find out whether the owner content really is a widget
> +   */
> +  readonly attribute boolean ownerIsWidget;

How does ownerIsWidget interact with owerIsBrowserOrAppFrame? Could they both be true? Document this.

::: dom/apps/tests/test_widget.html
@@ +81,5 @@
> +      function ok(p, msg) { \
> +        if (p) \
> +          sendAsyncMessage("OK", msg); \
> +        else \
> +          sendAsyncMessage("KO", msg); \

if () {
} else {
}

@@ +88,5 @@
> +      function is(a, b, msg) { \
> +        if (a == b) \
> +          sendAsyncMessage("OK", a + " == " + b + " - " + msg); \
> +        else \
> +          sendAsyncMessage("KO", a + " != " + b + " - " + msg); \

ditto

@@ +95,5 @@
> +      function finish() { \
> +          sendAsyncMessage("DONE",""); \
> +      } \
> +      \
> +      function cbError() { \

onError

::: dom/browser-element/BrowserElementParent.cpp
@@ +169,5 @@
> +  nsCOMPtr<nsIMozBrowserFrame> browserFrame =
> +    do_QueryInterface(aOpenerFrameElement);
> +  if (browserFrame && browserFrame->GetReallyIsWidget()) {
> +    return BrowserElementParent::OPEN_WINDOW_CANCELLED;
> +  }

Test this.

@@ +362,5 @@
> +  NS_ENSURE_TRUE(frameElement, false);
> +  nsCOMPtr<nsIMozBrowserFrame> browserFrame = do_QueryInterface(frameElement);
> +  if (browserFrame && browserFrame->GetReallyIsWidget()) {
> +    return true;
> +  }

Test this.

::: dom/browser-element/BrowserElementParent.jsm
@@ +119,2 @@
>  
> +  // Not expose security sensitive browser API for widgets

Each method in this list should be tested like getScreenshot.

@@ +275,5 @@
> +      } else if (self._isAlive() &&
> +                 (aMsg.data.msg_name in mmSecuritySensitiveCalls) &&
> +                 !isWidget ) {
> +        return mmSecuritySensitiveCalls[aMsg.data.msg_name]
> +                 .apply(self, arguments);

Check !self._isAlive() and bailout early in this function.
Attachment #8471468 - Flags: review?(kchen) → feedback+
Plan to land it by the end of v2.1 sprint 3 (August 29).
Target Milestone: --- → 2.1 S3 (29aug)
Assignee

Comment 45

5 years ago
Update according to feedback comment 43
Attachment #8471468 - Attachment is obsolete: true
Attachment #8472237 - Flags: review?(kchen)
Comment on attachment 8472237 [details] [diff] [review]
Part 2(v2): Only limited browser API are available to a widget

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

Looks good but I still have some questions

::: content/base/public/nsIFrameLoader.idl
@@ +285,2 @@
>     */
>    readonly attribute boolean ownerIsBrowserOrAppFrame;

Please add a comment in previous patch that says that nsIMozBrowserFrame::appManifestURL could return either the app manifest or widget manifest, otherwise the code in GetReallyIsApp and this comment is rather non intuitive.

::: content/html/content/src/nsGenericHTMLFrameElement.cpp
@@ +315,5 @@
> +{
> +  if (!Preferences::GetBool("dom.enable_widgets")) {
> +    *aOut = false;
> +    return NS_OK;
> +  }

Cache this like nsGenericHTMLFrameElement::BrowserFramesEnabled()

::: dom/interfaces/html/nsIMozBrowserFrame.idl
@@ +37,5 @@
> +   * frame (this requirement will go away eventually), the frame's mozwidget
> +   * attribute must point to the manifest of a valid app , and the src should
> +   * be in the |widgetPages| specified by the manifest.
> +   */
> +  [infallible] readonly attribute boolean reallyIsWidget;

Do we check the src is really in the |widgetPages|? Do we have tests for this?
Attachment #8472237 - Flags: review?(kchen)
Assignee

Comment 47

5 years ago
> ::: dom/interfaces/html/nsIMozBrowserFrame.idl
> @@ +37,5 @@
> > +   * frame (this requirement will go away eventually), the frame's mozwidget
> > +   * attribute must point to the manifest of a valid app , and the src should
> > +   * be in the |widgetPages| specified by the manifest.
> > +   */
> > +  [infallible] readonly attribute boolean reallyIsWidget;
> 
> Do we check the src is really in the |widgetPages|? Do we have tests for
> this?
We check the fact in nsGenericHTMLFrameElement::GetManifestURLByType.
I'll implement tests for this.

By the way, the behavour of invalid src will be changed in bug 1043110.
Assignee

Comment 48

5 years ago
Attachment #8474951 - Flags: superreview?(jonas)
Attachment #8474951 - Flags: review+
Assignee

Comment 49

5 years ago
(In reply to Junior Hsu [:juniorhsu] from comment #48)
> Created attachment 8474951 [details] [diff] [review]
> Part 1: Load a widget as an app if the |src| is in the |widgetPages|
Modify comments by comment 46, carry r+
Assignee

Comment 50

5 years ago
Modify by comment 46
Attachment #8468177 - Attachment is obsolete: true
Attachment #8468177 - Flags: superreview?(jonas)
Attachment #8474953 - Flags: review?(kchen)
Assignee

Comment 51

5 years ago
Comment on attachment 8472237 [details] [diff] [review]
Part 2(v2): Only limited browser API are available to a widget

obsolete owing to new patch
Attachment #8472237 - Attachment is obsolete: true
Comment on attachment 8474951 [details] [diff] [review]
Part 1: Load a widget as an app if the |src| is in the |widgetPages|

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

This looks mostly good to me.

However is there anything that ensures that the widget can't navigate to pages other than what's listed in widgetPages? This patch seems to make sure that we never open a page other than ones enumerated in manifest, but we should also make sure that if there's a link to other pages, that we prevent such navigation.

Or is that handled in a later patch?

Also, fabrice should probably look over the Webapps.jsm changes if he wants to.
Attachment #8474951 - Flags: superreview?(jonas)
Attachment #8474951 - Flags: superreview+
Attachment #8474951 - Flags: review?(fabrice)
(In reply to Junior Hsu [:juniorhsu] from comment #6)
> There's an ambiguous behaviour need to be clairfied.
> In the WIP patch, if the |src| is not listed in the |widgetPages| list, the
> iframe will be a general iframe, not a widget/app.
> Is the behaviour reasonable? Or it should be a blank page.

I think this is ok. It would be slightly cleaner to open a blank page, but if that's complex, then it seems ok to do what you're doing since it's a case that's unlikely to be hit commonly.
Flags: needinfo?(jonas)
Assignee

Comment 54

5 years ago
(In reply to Jonas Sicking (:sicking) from comment #52)
> Comment on attachment 8474951 [details] [diff] [review]
> Part 1: Load a widget as an app if the |src| is in the |widgetPages|
> 
> Review of attachment 8474951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks mostly good to me.
> 
> However is there anything that ensures that the widget can't navigate to
> pages other than what's listed in widgetPages? This patch seems to make sure
> that we never open a page other than ones enumerated in manifest, but we
> should also make sure that if there's a link to other pages, that we prevent
> such navigation.
> 
> Or is that handled in a later patch?
The navigation issue is tracked by bug 1043110.
The whole feature is under permission and preference, so IMO it's ok to handle it later.
> 
> Also, fabrice should probably look over the Webapps.jsm changes if he wants
> to.
fabrice has reviewed this patch, including Webapps.jsm.

Thanks for your help.
Sounds good. We should make sure to fix the navigation issue before turning the preference on.
Comment on attachment 8474953 [details] [diff] [review]
Part 2(v3): Only limited browser API are available to a widget

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

::: content/html/content/src/nsGenericHTMLFrameElement.cpp
@@ +328,5 @@
> +/* [infallible] */ NS_IMETHODIMP
> +nsGenericHTMLFrameElement::GetReallyIsWidget(bool *aOut)
> +{
> +  if (!nsGenericHTMLFrameElement::WidgetsEnabled()) {
> +    *aOut = false;

Put this line at the beginning of the function.

::: content/html/content/src/nsGenericHTMLFrameElement.h
@@ +90,5 @@
>    }
>  
>  private:
>    void GetManifestURLByType(nsIAtom *aAppType, nsAString& aOut);
> +  static bool WidgetsEnabled();

If you want to make this private then make it file local static function or put it into a anonymous namespace in .cpp

Otherwise make it public and alongside the BrowserFramesEnabled() declaration.
Attachment #8474953 - Flags: review?(kchen) → review+
Assignee

Comment 57

5 years ago
Rebase to m-c, carry r+, sr+
Attachment #8474951 - Attachment is obsolete: true
Attachment #8474953 - Attachment is obsolete: true
Attachment #8474951 - Flags: review?(fabrice)
Attachment #8475092 - Flags: superreview+
Attachment #8475092 - Flags: review+
Assignee

Comment 58

5 years ago
Rebase to m-c and modify based on comment 56, carry r+
Attachment #8475094 - Flags: review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
Flags: in-moztrap?(fyen)
Flags: in-moztrap-
No end-user QA verification needed, so marking verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.