Boris Zbarsky [:bzbarsky] Reporter
	Description • 7 years ago

STEPS TO REPRODUCE (on Mac): Open Tools > Web Developer menu

EXPECTED RESULTS: There are no "Enable Developer Tools..." items in the menu, because I'm on nightly and the "devtools.enabled" preference defaults to true.

ACTUAL RESULTS: There are lots of "Enable Developer Tools..." items in the menu.  Enough that I can't see anything else in the menu without scrolling the menu down, in fact.

ADDITIONAL INFORMATION:

1) I ran into this last week, but figured I had an old nightly, updated, restarted, and the problem seemed to be gone.  But it's clearly back now...  I only encounter this when I try to use a devtool I don't recall the keyboard shortcut for (e.g. scratchpad), so I don't know offhand whether this happens during startup or as I browse.  :(

2) document.querySelectorAll("#enableDeveloperTools").length evaluated in a chrome-environment scratchpad shows "68" as the value in the window I have focused right now.  It shows "1" in some other windows (e.g. ones restored at startup).

3) If I open 4 new windows with Cmd+N, each of them also reports "68" for the number of elements with that id....  So does the window I initially got the 68 in.

4) If I open a new window with the "open link in new window" context menu option (e.g. for the "Bugzilla@Mozilla" link at the top of this bug page), that window reports "69" for number of elements.

	Julian Descottes [:jdescottes] Assignee
	Comment 1 • 7 years ago

From the description I guess what happens here is that the "hookWindow" [1] callback here is called more than once per window.
This callback is attached to "browser-delayed-startup-finished". We expect to have it called only once per window. Maybe this assumption is incorrect?

I can easily update the code to make sure only one item gets created (in which case it should be correctly hidden later if devtools.enabled is false). I will submit a patch to do that for now.

I tried playing with creating new windows, either with cmd+N or "open link in new window", but I couldn't reproduce this yet. If you stumble upon the issue again, could you put a breakpoint in the body of [1] and see if this gets called more than once when you open a new window?

[1] http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/devtools/shim/devtools-startup.js#251

	Brian Grinstead [:bgrins]
	Comment 3 • 7 years ago
mozreview-review

Comment on attachment 8923913 [details]
Bug 1413246 - devtools-startup should add event listeners only on initial launch;

https://reviewboard.mozilla.org/r/195072/#review200112

I'm worried that the other calls in hookWindow will be duplicated as well. I'd prefer to know why hookWindow/onWindowReady is called more than once per-window but if we want to just assume that will happen then we should likely guard against this in onWindowReady (something like keep a WeakSet of windows and return early if it's already been processed)

	Julian Descottes [:jdescottes] Assignee
	Comment 4 • 7 years ago

At this stage I don't even know if hookWindow is called more than once, so I would prefer to have an actual confirmation from someone who can reproduce the problem the issue. That's why I proposed just treating the symptom for now, but I'm fine with waiting until we get more info too.

As to why we are calling this more than once (if this suggestion is correct), either we get the event several times or we run the handle() method of our startup component several times (and add several observers).

	Brian Grinstead [:bgrins]
	Comment 5 • 7 years ago

Boris, can you reproduce this problem in a local build as well?

	Brian Grinstead [:bgrins]
	Comment 6 • 7 years ago

(In reply to Julian Descottes [:jdescottes] from comment #4)
> At this stage I don't even know if hookWindow is called more than once, so I
> would prefer to have an actual confirmation from someone who can reproduce
> the problem the issue. That's why I proposed just treating the symptom for
> now, but I'm fine with waiting until we get more info too.
> 
> As to why we are calling this more than once (if this suggestion is
> correct), either we get the event several times or we run the handle()
> method of our startup component several times (and add several observers).

Even just for getting confirmation and getting a fix out, guarding inside onWindowReady seems safer (i.e. we won't end up with a slightly different problem due to a different function being called more than once). We could also add some extra logging via console.jsm inside of the DevToolsStartup constructor and/or the handle method if we detect it being called multiple times to help narrow down the problem.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 7 • 7 years ago

> could you put a breakpoint in the body of [1] and see if this gets called more than once
> when you open a new window?

Yes, it does.  It gets called a bunch of times.

What I can't tell you is how many times the code at http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/devtools/shim/devtools-startup.js#240 runs, and hence whether the problem is multiple notifications being sent, or multiple observers being registered.  Unfortunately, I haven't found a good way to backtrack the observers (which I can get a list of) to their underlying JS functions in a nightly.  And if I set a breakpoint on that line and try to do things that I think might hit it (e.g. clicking links in other applications), then I run into bug 1413309...

> Boris, can you reproduce this problem in a local build as well?

I don't know, because I do not have a reliable way to reproduce the problem...

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 8 • 7 years ago

OK, so I tried this, in a chrome scratchpad:

  var e = Services.obs.enumerateObservers("browser-delayed-startup-finished");
  var arr = [];
  while (e.hasMoreElements()) {
    arr.push(e.getNext())
  }
  arr.length

and I got 76. Then I clicked a link in my mail client, that opened a new tab in my browser with the page, etc.  I evaluated that same script again, I got 77.  I clicked links in my mail a few more times; every time I click it, I have one more "browser-delayed-startup-finished" observer.

And actually, I just tested this too:

1)  Evaluate:

  document.querySelectorAll("#enableDeveloperTools").length

in a new window and get 77.

2)  Click link in mail client.
3)  Open another new window, evaluate same thing, get 78.

So looks like handle() is called any time the OS opens a URL in Firefox due to it being the default browser, yes?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 9 • 7 years ago

I should note that this means each call to handle() leaks a bit too, and makes all new window creations slower, etc.

	Brian Grinstead [:bgrins]
	Comment 10 • 7 years ago

I see there are multiple states that a command line can be in: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICommandLine#State_constants.  Maybe we should only add observers and run setupEnabledPref if it's STATE_INITIAL_LAUNCH?

	Julian Descottes [:jdescottes] Assignee
	Comment 11 • 7 years ago

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #8)
> 
> So looks like handle() is called any time the OS opens a URL in Firefox due
> to it being the default browser, yes?

Thanks for the additional investigation! I reproduce this too if I open a link from another app and then open a new window.

(In reply to Brian Grinstead [:bgrins] from comment #10)
> I see there are multiple states that a command line can be in:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsICommandLine#State_constants.  Maybe we should only add
> observers and run setupEnabledPref if it's STATE_INITIAL_LAUNCH?

Nice find! Looks like it fixes the issue for me locally.

	Brian Grinstead [:bgrins]
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8923913 [details]
Bug 1413246 - devtools-startup should add event listeners only on initial launch;

https://reviewboard.mozilla.org/r/195072/#review200258

I don't know enough of the details about Ci.nsICommandLine to say if this will fix bz's problem, but this looks like a pretty safe change so if try is green we can try it and get confirmation that it did.

	Julian Descottes [:jdescottes] Assignee
	Comment 14 • 7 years ago

(In reply to Brian Grinstead [:bgrins] from comment #13)
> Comment on attachment 8923913 [details]
> Bug 1413246 - devtools-startup should add event listeners only on initial
> launch;
> 
> https://reviewboard.mozilla.org/r/195072/#review200258
> 
> I don't know enough of the details about Ci.nsICommandLine to say if this
> will fix bz's problem, but this looks like a pretty safe change so if try is
> green we can try it and get confirmation that it did.

Thanks for the review! It solves the problem I reproduced based on bz's info. 
My STRs are:
- open Firefox and check it is the default browser
- in the terminal (OSX) run `open http://mozilla.org`
- in Firefox open a new window
- open Tools > Web Developer
=> An "Enable Developer Tools" menu item will appear.

In this scenario, cmdLine.state is STATE_REMOTE_EXPLICIT (i.e. 2).

	Pulsebot
	Comment 15 • 7 years ago

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2a784d8fb27
devtools-startup should add event listeners only on initial launch;r=bgrins

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 16 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e2a784d8fb27