1639716 - Consider allowing profiling in private browsing sessions

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), enhancement, P3)

Description

[:Harald Kirschner :digitarald]

Private browsing is a convenient way for profiling as it resets cache/cookies and cuts out addons impact.

The reasoning that PB disables profiling historically is preventing the accidental profiling of private browsing sessions.

I would argue that the obvious risk isn't recording and viewing the profiles in the Profiler – but sharing the data.

With that in mind, more fine-grained mitigation would be to warn/confirm with users when uploading a profile recorded in PB. Ongoing work on single-tab recording combined with the existing sanitization (removal of hidden data) the risk is further mitigated for general audience.

Comment 1

[Greg Tatum [:gregtatum]]

Moving to popup improvements, as this is not part of the design spec.

Comment 2

[Florian Quèze [:florian]]

(In reply to :Harald Kirschner :digitarald from comment #0)

The reasoning that PB disables profiling historically is preventing the accidental profiling of private browsing sessions.

I would argue that the obvious risk isn't recording and viewing the profiles in the Profiler – but sharing the data.neral audience.

My impression is that the biggest risk is starting the profiler from a normal window when a Private browsing window that should have remained private was still in the background. I don't think starting the profiler from a private browsing window should be a problem, as long as the user is intentionally doing it.

Comment 3

[Julian Descottes [:jdescottes]]

Considering that the old performance panel works on Private Browsing sessions, and that web developers tend to use this regularly for development, we might want to address this before we enable the new panel by default.

Julien, Honza what do you think?

Comment 4

[Julien Wajsberg [:julienw]]

Yeah I believe we could make it work, but then we should probably record it in the profile metadata and show it in the frontend, especially when the user will share it. I believe there's a small bit of work.

Comment 5

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

Some comments / thoughts:

• I agree that we should support profiling of Private Browsing sessions
• The "Share Performance Profile" popup can show a warning when the profile contains data from Private Browsing Sessions
• [optional] The DevTools Performance panel could show the same (visually similar) warning informing the user that there are Private Browsing sessions running in the backround that will be included in profiling.
• Should the default settings in the Performance panel be "Web Developer" as opposed to "Firefox Platform"?

Also, the "Web Developer" settings isn't equal to "single-tab recording", correct? Do we have a bug for "single-tab recording"?
It feels like regular DevTools users are mostly interested in debugging the current tab, which automatically excludes any other running browser sessions/tabs.

Comment 6

[Julian Descottes [:jdescottes]]

The DevTools Performance panel could show the same (visually similar) warning informing the user that there are Private Browsing sessions running in the backround that will be included in profiling.

So from DevTools' perspective, the preferred option is to allow recording sessions with private browsing windows and just display a warning. We'll need some analysis for what needs to be done outside of the simple UI changes in the DevTools panel. Also need to check with other stakeholders that this is an acceptable change.

Should the default settings in the Performance panel be "Web Developer" as opposed to "Firefox Platform"?

Oh good point, we should file a bug for this!

Comment 7

[Julian Descottes [:jdescottes]]

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #5)

Also, the "Web Developer" settings isn't equal to "single-tab recording", correct? Do we have a bug for "single-tab recording"?
It feels like regular DevTools users are mostly interested in debugging the current tab, which automatically excludes any other running browser sessions/tabs.

Redirecting the question to Julien. I think it's correct that WebDeveloper differs from single-tab recording, even though by default the "active tab" should be selected in the profile (currently there is a small issue with this, see https://bugzilla.mozilla.org/show_bug.cgi?id=1704546). This view should be focused on the active tab used when profiling.

Do we plan to go beyond this "active tab" view?

Comment 8

[Julien Wajsberg [:julienw]]

Thanks Julian, I was gonna answer this actually.

Indeed we don't plan to do something different for now, we expect that the "active tab" view will work good enough. In the future we'll want a tab switcher in the Profiler UI, I believe that will make it even easier. I also believe we'll want a new sanitization process to remove other tab's information. But nothing that prevents us from releasing !

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #5)

Some comments / thoughts:

• The "Share Performance Profile" popup can show a warning when the profile contains data from Private Browsing Sessions

Doing that is what I outlined in comment 4. This isn't difficult but there's some work.

• [optional] The DevTools Performance panel could show the same (visually similar) warning informing the user that there are Private Browsing sessions running in the backround that will be included in profiling.

This is a lot easier, so I believe we should do this.

• Should the default settings in the Performance panel be "Web Developer" as opposed to "Firefox Platform"?

The default is "Firefox Platform" only in nightly or local builds, otherwise it's "Web Developer".
see https://searchfox.org/mozilla-central/rev/9dceacf3d761eb91237108ec438d64099a56f442/modules/libpref/init/all.js#850-857
Is that acceptable?

Also, the "Web Developer" settings isn't equal to "single-tab recording", correct? Do we have a bug for "single-tab recording"?
It feels like regular DevTools users are mostly interested in debugging the current tab, which automatically excludes any other running browser sessions/tabs.

(see above)

Comment 9

[Julian Descottes [:jdescottes]]

The default is "Firefox Platform" only in nightly or local builds, otherwise it's "Web Developer".

Makes sense to me, I think it's fine.
Actually yesterday I tested in DevEdition, where I probably had Web Developer by default. So I was a bit surprised when reading the question :)

Comment 10

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

(In reply to Julien Wajsberg [:julienw] from comment #8)

The default is "Firefox Platform" only in nightly or local builds, otherwise it's "Web Developer".
see https://searchfox.org/mozilla-central/rev/9dceacf3d761eb91237108ec438d64099a56f442/modules/libpref/init/all.js#850-857
Is that acceptable?

I agree, this makes sense, yes.
Thank you, Julien

Comment 11

[Julian Descottes [:jdescottes]]

ni? myself to make this move forward

Comment 12

[Julien Wajsberg [:julienw]]

I'm working on this now :-)

Comment 13

[Julien Wajsberg [:julienw]]

Attachment: Bug 1639716 - [profiler] Record the private browsing information in registered pages r=gerald

Comment 14

[Julien Wajsberg [:julienw]]

Attachment: Bug 1639716 - [profiler] Remove the profiler disabling by private browsing r=jdescottes!,gerald!

Depends on D129416

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: Bug 1639716 - [devtools performance] Add a property to the root actor's traits and pass all traits to the gInit function r=jdescottes!

Depends on D129417

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: Bug 1639716 - [devtools performance] Use the traits to control the call to the removed functionality r=jdescottes!

Depends on D129418

Comment 17

[Julien Wajsberg [:julienw]]

Attachment: Bug 1639716 - [devtools performance] Remove all actor code related to the profiler locking mechanism by private browsing r=jdescottes!

Depends on D129419

Comment 18

[Julien Wajsberg [:julienw]]

Attachment: Bug 1639716 - [devtools performance] Cleanup: pass the openAboutProfiling function from panel.js for more simplicity r=jdescottes!

Depends on D129420

Comment 19

[Julien Wajsberg [:julienw]]

Attachment: Bug 1639716 - [devtools performance] Capture profiles only in non-private windows r=jdescottes!

Depends on D129421

Comment 20

[Julien Wajsberg [:julienw]]

Attachment: Bug 1639716 - [profiler] Extract origin attributes out of the eTLD+1 and output them as thread properties r=gerald!

Depends on D129518

Comment 21

[Julien Wajsberg [:julienw]]

Attachment: Bug 1639716 - [profiler, network markers] Add information about private browsing in network markers r=valentin!,gerald!

Depends on D129818

Comment 22

[Julien Wajsberg [:julienw]]

Windows Try: https://treeherder.mozilla.org/jobs?repo=try&revision=26374704eb62bdf8999a01af8b540e7246ab120b
Github work (tests still missing): https://github.com/firefox-devtools/profiler/pull/3671

Comment 23

[Julien Wajsberg [:julienw]]

Attachment: Bug 1639716 - [utilityOverlay] Provide a callback in openLinkIn to report when a content browser is created r=gijs!

Depends on D129421

Comment 24

[Julien Wajsberg [:julienw]]

Attachment: Bug 1639716 - [devtools performance] Change a few test helpers so that the window can be specified r=jdescottes!

Depends on D133375

Comment 25

[Julien Wajsberg [:julienw]]

Attachment: Bug 1639716 - [profiler] Bump the format version to ensure our users get the latest version of the frontend when capturing private browsing data r=gerald!

Depends on D129819

Comment 26

[Julien Wajsberg [:julienw]]

Final try before landing: https://treeherder.mozilla.org/jobs?repo=try&revision=589f37548072c3a2d92e1c3a5878cb5544125960

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7b679fcef1f
[profiler] Record the private browsing information in registered pages r=gerald
https://hg.mozilla.org/integration/autoland/rev/b6301f4ca68c
[profiler] Remove the profiler disabling by private browsing r=jdescottes,gerald,devtools-backward-compat-reviewers
https://hg.mozilla.org/integration/autoland/rev/53b4be41c530
[devtools performance] Add a property to the root actor's traits and pass all traits to the gInit function r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/9aa662b8da5e
[devtools performance] Use the traits to control the call to the removed functionality r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/001820edfacc
[devtools performance] Remove all actor code related to the profiler locking mechanism by private browsing r=jdescottes,devtools-backward-compat-reviewers
https://hg.mozilla.org/integration/autoland/rev/b32fc155af27
[devtools performance] Cleanup: pass the openAboutProfiling function from panel.js for more simplicity r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/0585facaeb83
[utilityOverlay] Provide a callback in openLinkIn to report when a content browser is created r=Gijs
https://hg.mozilla.org/integration/autoland/rev/0747f5dd266b
[devtools performance] Change a few test helpers so that the window can be specified r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/3ea70d53ef4e
[devtools performance] Capture profiles only in non-private windows r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/a2e43e51b6ca
[profiler] Extract origin attributes out of the eTLD+1 and output them as thread properties r=gerald
https://hg.mozilla.org/integration/autoland/rev/de1c260f94f5
[profiler, network markers] Add information about private browsing in network markers r=valentin,gerald,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/a615447107f1
[profiler] Bump the format version to ensure our users get the latest version of the frontend when capturing private browsing data r=gerald

Comment 28

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/d7b679fcef1f
https://hg.mozilla.org/mozilla-central/rev/b6301f4ca68c
https://hg.mozilla.org/mozilla-central/rev/53b4be41c530
https://hg.mozilla.org/mozilla-central/rev/9aa662b8da5e
https://hg.mozilla.org/mozilla-central/rev/001820edfacc
https://hg.mozilla.org/mozilla-central/rev/b32fc155af27
https://hg.mozilla.org/mozilla-central/rev/0585facaeb83
https://hg.mozilla.org/mozilla-central/rev/0747f5dd266b
https://hg.mozilla.org/mozilla-central/rev/3ea70d53ef4e
https://hg.mozilla.org/mozilla-central/rev/a2e43e51b6ca
https://hg.mozilla.org/mozilla-central/rev/de1c260f94f5
https://hg.mozilla.org/mozilla-central/rev/a615447107f1

Comment 29

[Julien Wajsberg [:julienw]]

dev-doc-needed for developer-oriented documentation