1340366 - Remove privilege APIs for har-builder, har-collector and clipboard

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

Get rid of all XPCOM privilege APIs in order to make NetMonitor possible to run on content process.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard

Review commit: https://reviewboard.mozilla.org/r/113292/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113292/

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8838360 [details]
Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113292/diff/1-2/

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8838360 [details]
Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113292/diff/2-3/

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8838360 [details]
Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113292/diff/3-4/

Comment 5

[[e-mail]]

Comment on attachment 8838360 [details]
Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard

https://reviewboard.mozilla.org/r/113292/#review114776

Thanks for the patch, some issue need to be addressed

::: devtools/client/netmonitor/har/har-builder.js:17
(Diff revision 4)
>    getFormDataSections,
>    getUrlQuery,
>    parseQueryString,
>  } = require("devtools/client/netmonitor/request-utils");
>  
> -loader.lazyGetter(this, "L10N", () => {
> +const L10N = new LocalizationHelper("devtools/client/locales/har.properties");

please do so in `netmonitors/utils/l10n` for consistency, 

```
const HAR_STRINGS_URI = "devtools/client/locales/har.properties";
exports.HAR_L10N = new LocalizationHelper(HAR_STRINGS_URI);
```

::: devtools/client/netmonitor/har/har-collector.js:392
(Diff revision 4)
>      file.endedMillis = file.startedMillis + totalTime;
>    },
>  
>    // Helpers
>  
> -  getLongHeaders: makeInfallible(function (headers) {
> +  getLongHeaders: function (headers) {

Not sure if we should remove `makeInfallible` call, since debugger still use it somewhere so its fine to keep it

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8838360 [details]
Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113292/diff/4-5/

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8838360 [details]
Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard

https://reviewboard.mozilla.org/r/113292/#review114776

> please do so in `netmonitors/utils/l10n` for consistency, 
> 
> ```
> const HAR_STRINGS_URI = "devtools/client/locales/har.properties";
> exports.HAR_L10N = new LocalizationHelper(HAR_STRINGS_URI);
> ```

I know there exists a netmonitors/utils/l10n but I'd prefer not to use it because har/ could treat as a separate module from netmonitor (it belongs to its folder). As a result, I'd like to leave that as it is.

However, do you think har could be part of utils/ ? If that makes sense, we should move it to utils and merge l10n to utils/l10n as well.

> Not sure if we should remove `makeInfallible` call, since debugger still use it somewhere so its fine to keep it

It's merely a wrapper function for try catch and ignore any error.[1] It's safe. remove makeInfalliable can make us remove "devtools/shared/DevToolsUtils" as well. It would be easier to bring Netmonitor to Github since we don't have to care about bundling DevToolsUtils library (which is using a bunch of XPCOM APIs).

[1] http://searchfox.org/mozilla-central/source/devtools/shared/ThreadSafeDevToolsUtils.js#98

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Ricky Chien [:rickychien] from comment #7)
> Comment on attachment 8838360 [details]
> Bug 1340366 - Remove privilege APIs for har-builder, har-collector and
> clipboard
> 
> https://reviewboard.mozilla.org/r/113292/#review114776
> 
> > please do so in `netmonitors/utils/l10n` for consistency, 
> > 
> > ```
> > const HAR_STRINGS_URI = "devtools/client/locales/har.properties";
> > exports.HAR_L10N = new LocalizationHelper(HAR_STRINGS_URI);
> > ```
> 
> I know there exists a netmonitors/utils/l10n but I'd prefer not to use it
> because har/ could treat as a separate module from netmonitor (it belongs to
> its folder). As a result, I'd like to leave that as it is.
> 
> However, do you think har could be part of utils/ ? If that makes sense, we
> should move it to utils and merge l10n to utils/l10n as well.
I vote for keeping top level 'har' directory.

I like the idea of treating HAR implementation as separate independent
module. It's nice from architectural point of view but, I doubt that it
could be use independently from the Network monitor.
I think it's ok if the HAR module uses API from 'utils' (utils are
supposed to be shared). Anyway, I am open for any suggestions and we
can discuss at our meeting.

> 
> > Not sure if we should remove `makeInfallible` call, since debugger still use it somewhere so its fine to keep it
> 
> It's merely a wrapper function for try catch and ignore any error.[1] It's
> safe. remove makeInfalliable can make us remove
> "devtools/shared/DevToolsUtils" as well. It would be easier to bring
> Netmonitor to Github since we don't have to care about bundling
> DevToolsUtils library (which is using a bunch of XPCOM APIs).
Sounds good to me.

Honza

> 
> [1]
> http://searchfox.org/mozilla-central/source/devtools/shared/
> ThreadSafeDevToolsUtils.js#98

Comment 9

[[e-mail]]

Comment on attachment 8838360 [details]
Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard

https://reviewboard.mozilla.org/r/113292/#review114822

Since har-builder.js also used `request-utils` there's still some dependencies with netmonitor/, moving l10n to single reference might ease future tracing effort. But its fine to keep as it is

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8838360 [details]
Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard

https://reviewboard.mozilla.org/r/113292/#review114920

R+, but  please see my inline questions.


Honza

::: devtools/client/netmonitor/har/har-collector.js:400
(Diff revision 5)
> +        try {
> -        this.getString(header.value).then(value => {
> +          this.getString(header.value).then(value => {
> -          header.value = value;
> +            header.value = value;
> -        });
> +          });
> +        } catch (err) {
> +          // Skip failures

Shouldn't we have an error log here so, exceptions are not hidden?

::: devtools/client/netmonitor/netmonitor-controller.js:7
(Diff revision 5)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> -const promise = require("promise");
>  const Services = require("Services");

Do we have a plan for `const Services = require("Services");` ?

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8838360 [details]
Bug 1340366 - Remove privilege APIs for har-builder, har-collector and clipboard

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113292/diff/5-6/

Comment 12

[Ricky Chien [:rickychien] (inactive)]

(In reply to Jan Honza Odvarko [:Honza] from comment #10) 
> ::: devtools/client/netmonitor/har/har-collector.js:400
> (Diff revision 5)
> > +        try {
> > -        this.getString(header.value).then(value => {
> > +          this.getString(header.value).then(value => {
> > -          header.value = value;
> > +            header.value = value;
> > -        });
> > +          });
> > +        } catch (err) {
> > +          // Skip failures
> 
> Shouldn't we have an error log here so, exceptions are not hidden?

Done.

> 
> ::: devtools/client/netmonitor/netmonitor-controller.js:7
> (Diff revision 5)
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> >  "use strict";
> >  
> > -const promise = require("promise");
> >  const Services = require("Services");
> 
> Do we have a plan for `const Services = require("Services");` ?

I think we can leverage Services shim from debugger.html, but I haven't investigated it yet. I'd like to ask Jason on IRC to see his feedback.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ef8779085ae
Remove privilege APIs for har-builder, har-collector and clipboard r=gasolin,Honza

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3ef8779085ae