Closed Bug 1340366 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Iteration:
54.2 - Feb 20
Tracking Status
firefox54 --- fixed

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

Get rid of all XPCOM privilege APIs in order to make NetMonitor possible to run on content process.
Flags: qe-verify?
Blocks: 1340368
Iteration: --- → 54.2 - Feb 20
Priority: -- → P1
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
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
Attachment #8838360 - Flags: review?(gasolin)
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
(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 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
Attachment #8838360 - Flags: review?(gasolin) → review+
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");` ?
Attachment #8838360 - Flags: review?(odvarko) → review+
(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.
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ef8779085ae
Remove privilege APIs for har-builder, har-collector and clipboard r=gasolin,Honza
https://hg.mozilla.org/mozilla-central/rev/3ef8779085ae
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Whiteboard: [netmonitor-reserve] → [netmonitor]
Flags: qe-verify? → qe-verify-
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: