Closed
Bug 1460727
Opened 7 years ago
Closed 6 years ago
Let's not collect URLs for network loads by default
Categories
(Core :: Gecko Profiler, enhancement, P2)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox62 | --- | affected |
People
(Reporter: mstange, Assigned: canova)
References
Details
Attachments
(1 obsolete file)
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.
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → canaltinova
Status: NEW → ASSIGNED
Assignee | ||
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
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•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
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.
Reporter | ||
Comment 5•6 years ago
|
||
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.
Reporter | ||
Comment 6•6 years ago
|
||
mozreview-review |
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.
Attachment #8983976 -
Flags: review?(mstange) → review+
Reporter | ||
Comment 7•6 years ago
|
||
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.
Assignee | ||
Comment 8•6 years ago
|
||
It sounds fine by me. Will rename the name to collectnetworkurls in here and in the addon.
Assignee | ||
Updated•6 years ago
|
Comment 9•6 years ago
|
||
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.
Flags: needinfo?(mstange)
Reporter | ||
Comment 10•6 years ago
|
||
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.
Flags: needinfo?(mstange)
Comment 11•6 years ago
|
||
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?
Assignee | ||
Comment 12•6 years ago
|
||
This PR on perf.html side will anonymize just on share if the checkbox is not selected(which is the default option).
Assignee | ||
Updated•6 years ago
|
Attachment #8983976 -
Attachment is obsolete: true
Assignee | ||
Comment 13•6 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•