1460727 - Let's not collect URLs for network loads by default

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P2)

Description

[Markus Stange [:mstange]]

The new network markers contain URLs for all network loads. Including this information by default is probably the wrong call; users who report performance issues might not be aware of the full implications of this.

I think it would be better to have a "profiler feature" for network marker URLs, which is off by default, but can be turned on with a checkbox in the profiler add-on.

Comment 1

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1460727 - Add a profiler feature to not collect URLs for network loads by default

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

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

I tested this on my local with the profiler addon updated. It seems to be working fine. But when urls are empty, marker tooltips on the perf.html looks a bit visually shifted. That will probably require a small patch for perf.html too.
Also will do a try build, I don't know if there is a test for that or not.

Comment 3

[Julien Wajsberg [:julienw]]

Would it make sense to keep the last characters, so that we can know which "type" it is ? (with the caveat of get parameters getting in the way)

Comment 4

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

In nsIURI class, I see there is a `filePath` attribute that we can use for this. For example, if the URI is "http://host/foo/bar.html#baz" we get the "/foo/bar.html" with filePath attribute. I couldn't see an attribute to get the file extension only. We can use this attribute but I'm not sure if that really addresses the "not collect URLs" goal of this issue.

Comment 5

[Markus Stange [:mstange]]

I don't think trying to keep around the extension is worth doing. It complicates the code, it makes the privacy impact more complicated to state precisely, and it's unreliable because any file extension could come with any mime type.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8983976 [details]
Bug 1460727 - Add a profiler feature to not collect URLs for network loads by default

https://reviewboard.mozilla.org/r/249834/#review257096

::: tools/profiler/public/GeckoProfiler.h:106
(Diff revision 1)
>  // |macro| appropriately to extract the relevant parts. Note that the number
>  // values are used internally only and so can be changed without consequence.
>  // Any changes to this list should also be applied to the feature list in
>  // browser/components/extensions/schemas/geckoProfiler.json.
>  #define PROFILER_FOR_EACH_FEATURE(macro) \
>    /* Profile Java code (Android only). */ \

This comment needs to be updated.

Comment 7

[Markus Stange [:mstange]]

Maybe we should rename this feature to "collectnetworkurls". We have lots of other places where we collect URLs, and this feature doesn't affect those: JS function frames come with the URL of the JS or HTML file that they were in, and we have some label frames with URLs, e.g. PresShell::Paint and nsContentSink::StartLayout.

Comment 8

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

It sounds fine by me. Will rename the name to collectnetworkurls in here and in the addon.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

So, not collecting them by default makes profiles dramatically less useful to users looking at their own profiles...

I would prefer to capture them, but by default not export them on Share (arguable when Save to File).  Also, I'd replace them with placeholders: http://site1/url1.html/js (perhaps keep the the final .foo, maybe) instead of just removing them.

This would require us to implement options when sharing - i.e. the default would be Anonymize Network URLs; with Anonymize all URLs (or Fully-Anonymize - including function names) and Don't Anonymize.

Comment 10

[Markus Stange [:mstange]]

Hmm, I agree that such a solution would be preferable. I don't have a strong opinion on what we should do in short term. I'm still a bit afraid that unsuspecting users of the profiler could get their sessions hijacked.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Perhaps just anonymize or remove network URLs on Share, and let the UI for deciding to do anything different wait.  If you want to share a profile with network urls, use Save and use the file?

Comment 12

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

This PR on perf.html side will anonymize just on share if the checkbox is not selected(which is the default option).

Comment 13

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

The PR I mentioned is merged now. I think we can mark this bug as resolved. There are still things that we can do on the UX. It's described and will be tracked on that issue: https://github.com/devtools-html/perf.html/issues/1092