1386703 - Stop forcing the browser toolbox to use the new frontend

Status: RESOLVED INVALID

(DevTools :: Debugger, enhancement, P3)

Description

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

When the browser toolbox is opened, it copies the current profile to a new dedicated profile. It also forces some preferences, such as devtools.debugger.new-debugger-frontend to true.

While it made sense to force it to false while the new debugger could *not* be correctly used in the Browser Toolbox, forcing it to true now just means we prevent people from choosing between old / new debugger in the Browser Toolbox.

If some users find a bug in the new debugger and want to revert to the old debugger for some reason, this choice should apply to both the toolbox and the browser toolbox.

Comment 1

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

Attachment: Bug 1386703 - do not force new debugger for the browser toolbox;

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

Comment 2

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

Comment on attachment 8893007 [details]
Bug 1386703 - do not force new debugger for the browser toolbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164008/diff/1-2/

Comment 3

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

Tom: in this patch I'm blindly removing the forced pref on devtools.debugger.new-debugger-frontend, but I'm not sure what to do about the next one:

>  Services.prefs.setBoolPref("devtools.debugger.client-source-maps-enabled", true);

Do you think we should also rely on the current value from the user profile?

Comment 4

[Tom Tromey :tromey]

(In reply to Julian Descottes [:jdescottes] from comment #3)
> Tom: in this patch I'm blindly removing the forced pref on
> devtools.debugger.new-debugger-frontend, but I'm not sure what to do about
> the next one:
> 
> >  Services.prefs.setBoolPref("devtools.debugger.client-source-maps-enabled", true);
> 
> Do you think we should also rely on the current value from the user profile?

I tend to think we should, because then that gives users a chance to enable or disable
the pref, right?

I'm going to clear the r? on this basis; and this way I'll be notified when there's a new patch.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I believe the prefs are only copied to BT on _first_ open (when it creates a profile).  If the profile already exists, nothing is done, so it can still be challenging to change prefs in BT even with this patch.

Comment 6

[Tom Tromey :tromey]

Comment on attachment 8893007 [details]
Bug 1386703 - do not force new debugger for the browser toolbox;

https://reviewboard.mozilla.org/r/164008/#review169350

Clearing review since I think Julian is going to send a new patch.

Comment 7

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> I believe the prefs are only copied to BT on _first_ open (when it creates a
> profile).  If the profile already exists, nothing is done, so it can still
> be challenging to change prefs in BT even with this patch.

I suppose you are referring to this try/catch: http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/devtools/client/framework/ToolboxProcess.jsm#172-181

I was a bit puzzled by this when I modified this code for "ship devtools as a system addon" but we never actually seem to hit the catch.
So in my testing, I always end up copying the profile, every time I open the browser toolbox. Do you have a specific scenario in mind that would trigger the exception? (I am testing on OSX if that matters)

Comment 8

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

(In reply to Tom Tromey :tromey from comment #6)
> Comment on attachment 8893007 [details]
> Bug 1386703 - do not force new debugger for the browser toolbox;
> 
> https://reviewboard.mozilla.org/r/164008/#review169350
> 
> Clearing review since I think Julian is going to send a new patch.

Thanks for the review Tom! Waiting to clarify the point brought up by jryans before submitting a new patch. (And I should mention I touched a word about this bug to Jason, and he agreed to the concept of this modification)

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: BT Profile Logging

(In reply to Julian Descottes [:jdescottes] from comment #7)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > I believe the prefs are only copied to BT on _first_ open (when it creates a
> > profile).  If the profile already exists, nothing is done, so it can still
> > be challenging to change prefs in BT even with this patch.
> 
> I suppose you are referring to this try/catch:
> http://searchfox.org/mozilla-central/rev/
> bbc1c59e460a27b20929b56489e2e55438de81fa/devtools/client/framework/
> ToolboxProcess.jsm#172-181
> 
> I was a bit puzzled by this when I modified this code for "ship devtools as
> a system addon" but we never actually seem to hit the catch.
> So in my testing, I always end up copying the profile, every time I open the
> browser toolbox. Do you have a specific scenario in mind that would trigger
> the exception? (I am testing on OSX if that matters)

I am also testing on macOS.

I added more dump lines (patch attached) to trace exactly what happens in the common case (where you've already opened BT once before).

On my machine, I do indeed get the error `NS_ERROR_FILE_ALREADY_EXISTS` at the create step and we return from `initProfile` before saving the prefs.

How do you run Firefox when testing things locally?  Are you sure it's reusing the same profile in general?

As a test to see if prefs are copied, I used disabling the Perf tool as a test case.  My browser and content toolboxes both had it enabled.  I disabled it in the content toolbox, which sets devtools.performance.enabled to false.  I opened browser toolbox, and it was still enabled there, suggesting that no prefs were transferred.

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I guess once your patch is applied, the BT options screen is good enough to toggle the debugger, since it's not forced, which it sounds like it want you wanted...

Mainly I was just worried about comment 0 saying "When the browser toolbox is opened, it copies the current profile to a new dedicated profile", since that so far doesn't appear to be true, at least for me.

That's why we ended up with things in toolbox-process-window.js for BT, because we currently have no mechanism to set a "default" pref value specifically for BT.  Once someone opens BT the first time, it snapshots prefs and then never touches them.  So, we started forcing things in toolbox-process-window.js as a kind of hack when we were "pretty sure" most people would want a certain thing in BT without having to dig through options (and about:config is pretty hard to reach for the BT profile).

Comment 11

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

That's really weird. I'm running Firefox via ./mach run -profile ~/my-profile .

My test here was to (with my patch) :
- use the new debugger frontend
- start the BT -> BT uses new debugger frontend
- kill the BT
- use old debugger frontend by updating the pref
- start the BT -> BT uses old debugger frontend

Tried your scenario with the Performance tool as well, but I don't see the same output as you.
I have similar logs locally and I never seem to go in the catch ...

Comment 12

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> I guess once your patch is applied, the BT options screen is good enough to
> toggle the debugger, since it's not forced, which it sounds like it want you
> wanted...
> 

Sadly the BT options screen only displays the "new debugger frontend" option for Nightly and requires a restart of the toolbox (or requires you to not have clicked on the debugger tab before).

We have from time to time people coming to #devtools and asking for a way to have the old-debugger in the BT, and we only have complex workarounds to provide. Once the new debugger reaches a bigger audience I expect this kind of complaints to be more frequent so I just want to find a way to improve this.

Comment 13

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

As discussed on IRC the difference seems to come from the profile folder location.

When running with ./mach run -P profile, I get the behavior described by jryans, when running ./mach run -p ~/myprofile I get what I described earlier.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Julian Descottes [:jdescottes] from comment #13)
> As discussed on IRC the difference seems to come from the profile folder
> location.
> 
> When running with ./mach run -P profile, I get the behavior described by
> jryans, when running ./mach run -p ~/myprofile I get what I described
> earlier.

Running outside of the "regular" profile directory like :jdescottes triggers a bug in `_migrateProfileDir`, causing it to always delete the BT profile.  I filed bug 1386830 about this.

Comment 15

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

Closing as invalid, bug is not actionable given the way we create the BT profile. Plus we should focus on removing the old debugger.