Start Private Browsing Mode. Open console in the performance tab. Click start recording. Click stop recording. Results: Recording does not work, and afterwards you cannot close the devtools.

The profiler is disabled in private browsing (reasoning in bug 896222) -- but we should still have a better UI for this, and certainly should not hang devtools.

This has come up frequently -- we need to display a message saying it's disabled in private browsing, or if there's a window with private browsing. We can also disable the tool on the toolbox level, but it'll be harder to explain that succinctly.

Is this doable by next week's train? Bonus points if we can aurora-land it before then too, leaving only release with this. Right now it breaks the toolbox sometimes and sometimes works fine in private browsing -- either way, we should have some simple message atleast and prevent from toolbox breakage.

I think we can do it.

-_-

Comment on attachment 8659897 [details] [diff] [review] v3 Review of attachment 8659897 [details] [diff] [review]: ----------------------------------------------------------------- Does this check only if the current window is in private mode? I think we still have the same profiler issue if there exists a private window anywhere (not necessarily the window with the debuggee tab). Also, since this can fail when there exists a private window outside of the debuggee, since we only check on init, removing the private window will still leave this tool disabled. I'll do some tests to see if this is the case, but I think it is, even though sometimes it does seem to work. The pane for this message should either be generalized for any reason that the tool is unavailable, or the "unavailable" enum should be renamed everywhere to "privateBrowsing" or something -- right now it's specifically for PB, so we should change the enum IMO. ::: browser/devtools/performance/test/browser_perf-private-browsing.js @@ +17,5 @@ > + let panel = toolbox.getCurrentPanel(); > + let { $, PerformanceView } = panel.panelWin; > + > + is(PerformanceView.getState(), "unavailable", > + "The intial state of the performance panel view is correct."); s/intial/initial ::: toolkit/devtools/shared/profiler.js @@ +118,5 @@ > + config.features.length, > + config.threadFilters, > + config.threadFilters.length > + ); > + } catch (e) { We should log this error Cu.reportError so we can have some info if this fails for another reason

All good points, will address them asap.

Comment on attachment 8659897 [details] [diff] [review] v3 Actually, this works properly. It only checks if the current window is in private browsing mode. AFAICT everything is working properly.

Addressed comments. See https://bugzilla.mozilla.org/show_bug.cgi?id=1196947#c10

Comment on attachment 8660630 [details] [diff] [review] v4 Review of attachment 8660630 [details] [diff] [review]: ----------------------------------------------------------------- Just tested this out -- we need to test if there are any private tabs anywhere, not the current window -- all tabs are in the same process, regardless of window. Try this: * Normal e10s window, open tab 1, open perf tools * Open PB window and tab 2, open perf tools (disabled message) * Go back to tab 1 and attempt to record; failure * Open a new tab (3) in normal e10s window; no disabled message; failure It looks like we need to react any private tab existing (an event we can hook into? If not, we can lazily check when attempting to record). Also, the message is "recording is unavailable" with no explanation as to why still (should explicitly mention private browsing) -- it seems some users have private windows open as part of their normal browsing experience, and when debugging (in non PB window), perf tools just breaks the toolbox, being very confusing.

Remove dump statement

Comment on attachment 8661217 [details] [diff] [review] v6312341 Review of attachment 8661217 [details] [diff] [review]: ----------------------------------------------------------------- Few more comments.. ::: browser/devtools/performance/performance-controller.js @@ +209,5 @@ > + * Checks whether or not a new recording is supported by the PerformanceFront. > + * @return Promise:boolean > + */ > + canCurrentlyRecord: Task.async(function*() { > + let actorCanCheck = yield gTarget.actorHasMethod("performance", "canCurrentlyRecord"); We should make sure that this still works when there is no performance actor (silent fail) ::: browser/devtools/performance/performance-view.js @@ +204,5 @@ > * on `activate`. > * > * @param {boolean} activate > */ > + _flipRecordButtons: function (activate) { Let's use `toggle` rather than `flip`, I think that's more descriptive/universal. ::: browser/locales/en-US/chrome/browser/devtools/performance.dtd @@ +36,5 @@ > > <!-- LOCALIZATION NOTE (performanceUI.loadingNotice): This is the label shown > + - in the details view while the profiler is unavailable, for example, while > + - in Private Browsing mode. --> > +<!ENTITY performanceUI.unavailableNotice "Recording a profile is currently unavailable. Please close all private browsing windows and try again."> Should be `unavailableNoticePB` or something, incase in the future we have another reason why recording is unavailable. ::: toolkit/devtools/performance/recorder.js @@ +286,5 @@ > + * Checks whether or not recording is currently supported. At the moment, > + * this is only influenced by private browsing mode and the profiler. > + */ > + canCurrentlyRecord: function() { > + return Profiler.canProfile(); We should probably return an object here with some kind of message and success state -- we could have other scenarios where we cannot record ::: toolkit/devtools/server/actors/performance.js @@ +126,5 @@ > }), { > request: { > options: Arg(0, "nullable:json"), > }, > + response: { started: RetVal("boolean") } I have a feeling this will break a few things in tests, can we return `null` or something if it fails instead, and still return performance-recording? ::: toolkit/devtools/shared/profiler.js @@ +121,5 @@ > + ); > + } catch (e) { > + // For some reason, the profiler couldn't be started. This could happen, > + // for example, when in private browsing mode. > + Cu.reportError(`Could not start the profiler module: ${e.message}`); I wonder if we should bubble up errors here, and also check for private browsing here as well -- can emit an error event that's handled on the client and display whatever string message it is. Although this is for other errors, maybe can expand this in another bug. ::: tools/profiler/gecko/nsProfiler.cpp @@ +70,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +nsProfiler::CanProfile(bool *aCanProfile) Nice

This was insane.

Good enough: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9760c8471f64

Comment on attachment 8665817 [details] [diff] [review] v6312344 Review of attachment 8665817 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/performance/performance-controller.js @@ +413,5 @@ > + // the `NEW_RECORDING` event won't be handled anywhere, meaning that it will > + // never show up in the UI. > + // Furthermore, the Toolbox is directly responsible with adding all the > + // console recordings after the whole frontend is initialized. > + if (!gFrontendInitialized) { Do we have to do this? Ughhhhh. The toolbox should handle populating recordings created before the tool is spun up, and not sure why it doesn't solve this issue... Ok, rereading the comment more, I see. I had an assumption that the controller does not finish spinning up until all the views are ready -- can we do that instead, pushing the complexity into the lazy loading function in the toolbox? Right now we wait for the panel.open() method to complete -- can we tie all the subview children to be completed before that's done? I thought our chain of initializer functions waited for its children to finish before resolving, but maybe we're missing a yield or something somewhere. That'd be preferable for sooooo many reasons.

More on above: Once we ensure our cascading initializers are resolving iff their children are initialized, this should "just work" -- right now we wait for `panel.open()` in the lazy loading situation[0], and this really shouldn't be done before everything internally is done, I think that is a good assumption to have, and it's strange that that is not the case currently. If the initializer chain does not solve this, then we should wait for some other event or status in this onPerformanceFrontEvent in the toolbox, as we shouldn't (and don't currently) handle an "incompletely loaded" scenario anywhere else in the tool (for terrible, obvious reasons that we now both understand, in a sad, deep, intimately horrifying way) [0] https://github.com/mozilla/gecko-dev/blob/a1b0ef663bc1c8ff514e7b228f77fe64b88e6f7b/devtools/client/framework/toolbox.js#L2074

Comment on attachment 8665817 [details] [diff] [review] v6312344 Review of attachment 8665817 [details] [diff] [review]: ----------------------------------------------------------------- We talked on IRC about handling this by turning on the controller's listeners after both controller and views are inited, r+

Addressed comments and landed.

I have successfully reproduced this bug on Firefox Nightly Version 43.0a1 (2015-08-20) It's fixed and verified on Latest Nightly Build ID 20151002030204 User Agent Mozilla/5.0 (Windows NT 6.3; rv:44.0) Gecko/20100101 Firefox/44.0 Tested OS- Windows 8.1 32bit

Successfully reproduced this bug by following comment 0 with Firefox Nightly 43.0a1 (2015-08-20); (Build ID: 20150820030202) on Linux, 64 Bit This Bug is now verified as fixed on Latest Firefox Dev Edition 44.0a2 (2015-11-26) Build ID: 20151126004035 User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0 As it is also verified on Windows (Comment 26), Changing the status!