Closed Bug 1386821 Opened 7 years ago Closed 7 years ago

Developer toolbar not displayed when firefox first starts up

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

56 Branch
defect
Not set
normal

Tracking

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 verified, firefox57 verified)

VERIFIED FIXED
Firefox 57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- verified
firefox57 --- verified

People

(Reporter: wlach, Assigned: jdescottes)

References

Details

(Keywords: regression)

Attachments

(3 files)

Reproduction steps:

Start firefox with `devtools.toolbar.visible` set to TRUE.

Expected:

The "developer toolbar" (shift-f2) is present on completed startup. 

Actual:

The "developer toolbar" (shift-f2) is not present. If I browse into tools->web developer, it suddenly appears.

Doing a bisection with mozregression (mozregression --pref "devtools.toolbar.visible:true" -g 2017-07-01 -b 2017-08-01), I got this pushlog:

 7:42.83 INFO: Last good revision: f91d86bb4b2adad09711ce55c82c8d695b8e6ed7
 7:42.83 INFO: First bad revision: 1b4dea0fe0213d36833103764921046ec41bcca6
 7:42.83 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f91d86bb4b2adad09711ce55c82c8d695b8e6ed7&tochange=1b4dea0fe0213d36833103764921046ec41bcca6

It appears as if bug 1359855 is the culprit. Alexandre, any ideas?
Flags: needinfo?(poirot.alex)
This pref is used for redisplaying the developer toolbar on every browser restart, so we might want to uplift this.

Also we have been discussing about adding an entry point test for this kind of issues.
Alex: I think we need to start Firefox from the test in order to assert things such as devtools.toolbar.visible. Do you know if this is possible?
(In reply to Julian Descottes [:jdescottes] from comment #2)
> This pref is used for redisplaying the developer toolbar on every browser
> restart, so we might want to uplift this.
> 
> Also we have been discussing about adding an entry point test for this kind
> of issues.
> Alex: I think we need to start Firefox from the test in order to assert
> things such as devtools.toolbar.visible. Do you know if this is possible?

Not that I am aware of. mochitests always open a first browser window.
And xpcshell doesn't involve devtools-startup, nor would it support opening a browser window.
May be Talos addons can do that, but that's not really the right purpose.

In most cases, we test this kind of behavior by opening a new browser window:
http://searchfox.org/mozilla-central/source/devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js#50-51
  let win = OpenBrowserWindow();
  yield waitForDelayedStartupFinished(win);
That wouldn't ensure that the behavior is correct at startup,
but that should cover this code. You should be able to check somehow that initDevTools is called when opening a new window when the pref is on...
Flags: needinfo?(poirot.alex)
Comment on attachment 8893233 [details]
Bug 1386821 - call initDevTools if devtools.toolbar.visible is true;

https://reviewboard.mozilla.org/r/164274/#review169658

::: devtools/client/devtools-startup.js:196
(Diff revision 1)
>  
>      // Only top level Firefox Windows fire a browser-delayed-startup-finished event
>      let onWindowReady = window => {
>        this.hookWindow(window);
>  
> +      if (Services.prefs.getBoolPref(TOOLBAR_VISIBLE_PREF, false)) {

This will throw if the pref is moved to the add-on and doesn't exists by default, should you also move this pref?
Or do such safe check before calling getBoolPref:
if (Services.prefs.getPrefType(pref) != Services.prefs.PREF_INVALID && getBoolPref)

::: devtools/client/devtools-startup.js:369
(Diff revision 1)
>     * By initialized, we mean that its main modules are loaded.
>     */
>    initialized: false,
>  
>    initDevTools: function (reason) {
>      if (!this.initialized) {

If you don't pass a reason from onWindowReady, you should change this check here to:
  if (!this.initalized && reason) {
Attachment #8893233 - Flags: review?(poirot.alex) → review+
Assignee: nobody → jdescottes
Comment on attachment 8893233 [details]
Bug 1386821 - call initDevTools if devtools.toolbar.visible is true;

https://reviewboard.mozilla.org/r/164274/#review169658

> This will throw if the pref is moved to the add-on and doesn't exists by default, should you also move this pref?
> Or do such safe check before calling getBoolPref:
> if (Services.prefs.getPrefType(pref) != Services.prefs.PREF_INVALID && getBoolPref)

Since we pass a default value, this call should never throw, even if the pref is not defined.
If devtools are not installed, then it's ok to get the "false" default value. 

The question is, if devtools are installed, do we process the devtools prefs in the addon before doing this check or not.
I think you're right and we should simply be safe and move the pref to the startup preferences here. I'll block this on Bug 1386620

> If you don't pass a reason from onWindowReady, you should change this check here to:
>   if (!this.initalized && reason) {

good catch!
Depends on: 1386620
(In reply to Julian Descottes [:jdescottes] from comment #5)
> Comment on attachment 8893233 [details]
> Bug 1386821 - call initDevTools if devtools.toolbar.visible is true;
> 
> https://reviewboard.mozilla.org/r/164274/#review169658
> 
> > This will throw if the pref is moved to the add-on and doesn't exists by default, should you also move this pref?
> > Or do such safe check before calling getBoolPref:
> > if (Services.prefs.getPrefType(pref) != Services.prefs.PREF_INVALID && getBoolPref)
> 
> Since we pass a default value, this call should never throw, even if the
> pref is not defined.
> If devtools are not installed, then it's ok to get the "false" default
> value. 

Oh. I didn't knew that getBoolPref now support a default value. That's handy!
Comment on attachment 8893233 [details]
Bug 1386821 - call initDevTools if devtools.toolbar.visible is true;

https://reviewboard.mozilla.org/r/164274/#review169658

> good catch!

I decided to move this.initialized = true inside of the if (reason && !initialized) block.

The reasoning is that for devtools.toolbar.visible = true, even though it requires devtools tobe initialized, it's not really an interesting entry point. Even if the user has the toolbar opening by default we still want to know how they actually open the toolbox. 

Let me know if you're ok with that.
(In reply to Julian Descottes [:jdescottes] from comment #8)
> Comment on attachment 8893233 [details]
> Bug 1386821 - call initDevTools if devtools.toolbar.visible is true;
> 
> https://reviewboard.mozilla.org/r/164274/#review169658
> 
> > good catch!
> 
> I decided to move this.initialized = true inside of the if (reason &&
> !initialized) block.

You goal makes sense, but you can't modify `initialized` behavior like this.
First, the name of this property would be very misleading, but it also breaks its usage over here:
http://searchfox.org/mozilla-central/source/devtools/client/devtools-startup.js#211-214
Please introduce yet another flag to do what you want regarding telemetry here.
(In reply to Alexandre Poirot [:ochameau] from comment #9)
> (In reply to Julian Descottes [:jdescottes] from comment #8)
> > Comment on attachment 8893233 [details]
> > Bug 1386821 - call initDevTools if devtools.toolbar.visible is true;
> > 
> > https://reviewboard.mozilla.org/r/164274/#review169658
> > 
> > > good catch!
> > 
> > I decided to move this.initialized = true inside of the if (reason &&
> > !initialized) block.
> 
> You goal makes sense, but you can't modify `initialized` behavior like this.
> First, the name of this property would be very misleading, but it also
> breaks its usage over here:
> http://searchfox.org/mozilla-central/source/devtools/client/devtools-startup.
> js#211-214
> Please introduce yet another flag to do what you want regarding telemetry
> here.

Ah missed this usage of this.initialized. Since it was defined in just above initDevTools() I assumed it was only used there. Moved this.initialized back out of the if, and adding a second flag in another changeset.
Comment on attachment 8893340 [details]
Bug 1386821 - add a separate flag to check if devtools entry point was recorded;

https://reviewboard.mozilla.org/r/164434/#review170806

::: devtools/shim/devtools-startup.js:177
(Diff revision 2)
> +  initialized: false,
> +
> +  /**
> +   * Boolean flag to check if the devtools initialization was already sent to telemetry.
> +   * We only want to record one devtools entry point per Firefox run, but we are not
> +   * interested in all the entry points (e.g. devtools.toolbar.visible.

nit: missing `)` at the end.
Attachment #8893340 - Flags: review?(poirot.alex) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da2bc55ad33a
call initDevTools if devtools.toolbar.visible is true;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/e38283010bb3
add a separate flag to check if devtools entry point was recorded;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/da2bc55ad33a
https://hg.mozilla.org/mozilla-central/rev/e38283010bb3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Please nominate this for Beta approval when you get a chance.
Flags: needinfo?(jdescottes)
Version: unspecified → 56 Branch
Comment on attachment 8893233 [details]
Bug 1386821 - call initDevTools if devtools.toolbar.visible is true;

Approval Request Comment

[Feature/Bug causing the regression]: Bug 1359855

[User impact if declined]: The devtools command line is no longer re-opening on startup, forcing users to reenable it every time.

[Is this code covered by automated tests?]: no

[Has the fix been verified in Nightly?]: not yet

[Needs manual test from QE? If yes, steps to reproduce]: 
yes. STRs:
- open Firefox 
- open DevTools Command Line (SHIFT+F2)
- quit/re-open Firefox

[List of other uplifts needed for the feature/fix]: 
(we should uplift the patch from Bug 1387359 first to avoid conflicts!)
The other patch of this bug should also be uplifted, otherwise this patch will negatively impact the data collected by one of our telemetry probes.

[Is the change risky?]: no

[Why is the change risky/not risky?]: Small javascript fix, only impacting Devtools GCLI users.

[String changes made/needed]: none
Flags: needinfo?(jdescottes)
Attachment #8893233 - Flags: approval-mozilla-beta?
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
(In reply to Gerry Chang [:gchang] from comment #22)
> Hi Brindusa, could you help find someone to verify if this issue was fixed
> as expected on the latest Nightly build? Thanks!

FWIW I can verify that I am no longer seeing the issue in today's (2017-08-14) nightly.
Comment on attachment 8893233 [details]
Bug 1386821 - call initDevTools if devtools.toolbar.visible is true;

We want dev tools users to consistently see the toolbar. Fix verified, let's uplift for beta 3.
Attachment #8893233 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8893340 [details]
Bug 1386821 - add a separate flag to check if devtools entry point was recorded;

Note, we need the patch in bug 1387359 to land first.
Attachment #8893340 - Flags: approval-mozilla-beta+
Needs a rebased patch for Beta.
Flags: needinfo?(jdescottes)
This patch is rebased on the latest beta and merges both patches that landed on central.

I can't build beta locally for some reason, so let's wait for try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06a2d1f426590c2e4b1ea4b7a60df1179021baae

If try is green I will request for uplift again.
Flags: needinfo?(jdescottes)
Attachment #8897774 - Flags: review+
Try looks green except for unrelated intermittents. 
Can we use attachment 8897774 [details] [diff] [review] for the beta uplift?
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Flags: qe-verify+
I managed to reproduce the issue on an older Nightly 57.0a1 (2017-08-03) build under Windows 10 x64.
Verified as fixed using latest Nightly 57.0a1 (2017-08-17) on Ubuntu 16.04 x64, Windows 10 x64 and Mac OS X 10.12.6.
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
I also verified that this is fixed on Firefox 56 beta 9 using Windows 10 x64 and Ubuntu 16.04 x64.
Flags: qe-verify+
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.