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

RESOLVED FIXED in Firefox 54

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: rickychien, Assigned: rickychien)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 54
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [netmonitor])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

11 months ago
Blocks: 1307743
Flags: qe-verify?
(Assignee)

Updated

11 months ago
Blocks: 1340368
Comment hidden (mozreview-request)

Updated

11 months ago
Iteration: --- → 54.2 - Feb 20
Priority: -- → P1
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

11 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 7

11 months ago
mozreview-review-reply
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 9

11 months ago
mozreview-review
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 10

11 months ago
mozreview-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+
Comment hidden (mozreview-request)
(Assignee)

Comment 12

11 months ago
(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

11 months ago
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

Comment 14

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3ef8779085ae
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54

Updated

11 months ago
Whiteboard: [netmonitor-reserve] → [netmonitor]

Updated

11 months ago
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.