962478 - highlighter event listeners not removed when toolbox closed with highlighter visible.

Status: VERIFIED FIXED

(DevTools :: Inspector, defect)

Description

[Girish Sharma [:Optimizer]]

Open inspector and start the highlighter, do not pick any node and close the toolbox. Now any of the mouse events will not work on the page. You will have to close and open the tab again.

Patrick tells me that the highlighter's click event listener is not removed and that is what is stopping any mouse events to propagate to the page.

Comment 1

[Patrick Brosset <:pbro>]

Thanks for spotting that bug, and yes, it's due to our highlighter actor starting mouse listeners in the content window, and never having a chance to remove them.
I think this a little bit similar to bug 959632 in that, in both cases, we want the client to tell the highlighter to stop listening to mouse events on the page.
For now we only do that if you do pick an element in the page, or if you click again on the highlighter icon.

Comment 2

[Patrick Brosset <:pbro>]

This can be a good first bug. If I'm not mistaken, all we should do is add a step in the toolbox's close process.
This takes place in the toolbox's destroy method so, somewhere in there, we should add a check to see if the highlighter is in "pick mode" and if yes, stop it by sending a request to the server to do so.

Comment 3

[Girish Sharma [:Optimizer]]

yes, it is a good candidate. But just make sure that this gets fixed in this release as it is an annoying regression.

Comment 4

[Vignesh Shanmugam]

I wanted to contribute to this bug. Can you assign it to me. 

I tried reproducing this bug, when I open the inspector, the body element is getting selected deafult and when we close the Inspector with highlighter started it should end it on close. But its not behaving in the right way.

Can you guide me how to get the codebase? So that I can start digging in to it?

Comment 5

[Patrick Brosset <:pbro>]

I'm assigning the bug to you now.

Just to make sure we're on the same page as to what the problem is:
- Open the inspector on any page.
- Click on the inspect icon (the one that has the tooltip "Pick an element from the page", top right of the toolbox).
- Move your mouse over the page: you should see the highlighter follow your mouse.
- Now, click on the toolbox close icon (top left of the toolbox).
- Now try to click on any active elements of the page (a button or a link for instance)
==> The click events are not getting through at all.

Now, in order to get the codebase, build firefox and start modifying the code, you should start here:
https://wiki.mozilla.org/DevTools/GetInvolved
then here:
https://wiki.mozilla.org/DevTools/Hacking
And there are lot's of very important videos here:
http://codefirefox.com/

In terms of files that are important for this bug:
Most (if not all) code should be in toolbox.js:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js
The Toolbox class has a destroy function which is called when the toolbox is closed, it's in this function that you should stop the highlighter if it is running.

Comment 6

[Vignesh Shanmugam]

Sure, I will start working on it :)

Comment 7

[Girish Sharma [:Optimizer]]

Note that you will also have to do something similar to what is being done in https://bugzilla.mozilla.org/show_bug.cgi?id=961771

Comment 8

[Vignesh Shanmugam]

Attachment: Added the StopPicker method on the destoryInspector method;

Comment 9

[Vignesh Shanmugam]

Oops sorry for that, please consider only the toolbox.js file.

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8365486 [details] [diff] [review]
Added the StopPicker method on the destoryInspector method;

Review of attachment 8365486 [details] [diff] [review]:
-----------------------------------------------------------------

Vignesh, please do not set "review+" yourself. When asking for a review, you're supposed to set review? and add a reviewer's name in the field that appears.
And you will need to attach a new patch that does not contain the 2 mozconfig files.

::: browser/devtools/framework/toolbox.js
@@ +1061,5 @@
>      if (this._inspector) {
>        this._selection.destroy();
>        this._selection = null;
> +      if(this._isPicking){
> +          this.stopPicker();

stopPicker is async and returns a promise, so we want to wait until that completes before moving on with the rest of the function.

I think a better course of action would in fact be to move the |if (this._isPicking)| check inside stopPicker, and make the function always return a promise, even if _isPicking is false.

This would then allow us here, in destroyInspector, to simply do something like 
this.stopPicker().then(() => {
  this._walker.release().then(.... //and the rest ...
})

@@ +1082,5 @@
>        });
>      } else {
>        deferred.resolve();
>      }
> +   

Trailing whitespace here

Comment 11

[Vignesh Shanmugam]

Attachment: bug962478-corrected.patch

Made the stopPicker to return a promise always and moved the walker code once the stopPicker function has finished executing.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8365537 [details] [diff] [review]
bug962478-corrected.patch

Review of attachment 8365537 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there, the code looks ok, but you attached a patch which has been created on top of your first attached patch. So the diff we see here is the diff between your 2 patches, not between your patch and the code in mozilla-central (which it should be). Make sure you have an up-to-date version of mozilla-central (hg pull -u), and make sure you apply 1 patch only on top of it (see https://developer.mozilla.org/fr/docs/Mercurial_Queues).

::: browser/devtools/framework/toolbox.js
@@ +1080,5 @@
>        });
>      } else {
>        deferred.resolve();
>      }
>     

Trailing whitespace should be removed.
If you use SublimeText, there's an option there to remove them automatically everytime you save. I'm pretty sure other editors/IDEs have that option too.

@@ +1153,5 @@
>     * @return A promise that resolves when the picker has stopped
>     */
>    stopPicker: function() {
> +    if(!this._isPicking){
> +      return Promise.resolve();

I think it rather should be |promise.resolve()| (lowercase promise).
Does it work with the capital P?

Comment 13

[Patrick Brosset <:pbro>]

Oh, and one more thing I forgot to mention, it would be good to also do what you did in stopPicker in startPicker too.
So, checking if this._isPicking isn't true already, and if it is, return a promise.resolve().
This way it's safe to call these 2 methods at any time, and a consumer can be sure to always get a promise returned.

Comment 14

[Vignesh Shanmugam]

Attachment: bug962478-Final1.patch

Patrick, Thanks for the feedback.

I have removed the trailing whitespace on the destoryInspector method and Have added the same check in the startPicker method also.

I have updated the code and provided fix on top of that now. Hope it is good now :).

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8366012 [details] [diff] [review]
bug962478-Final1.patch

Review of attachment 8366012 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, just one mistake in |startPicker|.
Did you actually test the code? Because with that bug in there, the picker doesn't work anymore at all. Make sure you test your changes before submitting a patch, this often avoids attaching incorrect patches and wasting time on reviews.

How do you feel about writing automatic tests? Have you already taken a look at how they work? 
We try to test every patch we submit, so here, it would be good to write a test that:
- opens the toolbox
- starts the picker
- destroys the toolbox
- verifies that the picker is stopped

About tests:
- We have several types of tests, but one that suits this usecase nicely is the mochitest-browser type of test. This is an integration type of test where the browser is started and you get to manipulate its UI (via javascript) as a user would do it (https://developer.mozilla.org/en-US/docs/Mochitest and https://developer.mozilla.org/en-US/docs/Browser_chrome_tests).
- You can create a new mochitest-browser test file and place it into the /browser/devtools/inspector/test/ folder.
- It should be referenced in browser.ini and its name should start with browser_.
- It should contain a single |test| function, and call |waitForExplicitFinish()| to be able to finish the test asynchronously after the |test| function has returned, and call |finish()| when it's done.
- head.js in the inspector/test directory is loaded in the test's scope and contains all sorts of useful functions to ease the writing of the test.
- You have several assertions functions at your disposal: is, ok, isnot, ...
- Running the test goes like: ./mach mochitest-browser browser/devtools/inspector/test/name_of_your_test.js
- You can (and should) also run some/all tests by specifying a parent dir: ./mach mochitest-browser browser/devtools/inspector/ or even ./mach mochitest-browser browser/devtools/

A test was recently created on the same topic, maybe you can take a look at it to get a feeling of what you'll need to write. See http://mxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/test/browser_inspector_bug_961771_picker_stops_on_tool_select.js

Let me know if you need more help.

::: browser/devtools/framework/toolbox.js
@@ +1107,5 @@
>     * are hovered.
>     * @return A promise that resolves when the picker has started
>     */
>    startPicker: function() {
> +    if(!this._isPicking){

It should be the opposite: |if (this._isPicking) {|
Also, make sure to format your code as the rest of the file:
|if<space>(this._isPicking)<space>{|

Comment 16

[Vignesh Shanmugam]

Oops sorry for the mistake Patrick, I forgot to change that and sent the patch. Will update it and send.

I have not written any tests before. But I will read through the stuffs and try to do that and send you the patch. 

Thanks for your help.

Comment 17

[Patrick Brosset <:pbro>]

Have you had the chance to progress on this bug?
I'm sorry I may have missed you on IRC this week.
Please let me know if you need help progressing. It'd be good to get this bug in for Aurora.

Comment 18

[Vignesh Shanmugam]

I am trying to write the tests. Finding difficulty in it. might need help from you.

Comment 19

[Patrick Brosset <:pbro>]

So, as far as tests go, my advice is to look at other tests that have been written. It's the best way to get started.

For this one, I would start with the following:
- Create a new test file in /browser/devtools/inspector/test
- Name it something like browser_inspector_bug_962478_picker_stops_on_destroy.js (the browser prefix is important so the test is picked up by the test-runner)
- Open /browser/devtools/inspector/test/browser.ini and add a new entry with the test name at the end [browser_inspector_bug_962478_picker_stops_on_destroy.js]

The basics of a test file is that each test must declare a global test function which will get executed by the test-runner.

Once you know that, there are lot's of ways to write the actual test function content, but we try as much as possible to follow the same patterns.
You should take a look at test browser/devtools/inspector/test/browser_inspector_bug_961771_picker_stops_on_tool_select.js which has been written recently and which does almost what you want.
You will see 5 main parts in this file (all of which are inside the test function):

- importing dependencies that are needed to run this test (Cu.import and require stuff),
- executing waitForExplicitFinish() to signal the test runner that it should wait until the test calls finish() when it's done. This is important for asynchronous tests (which is basically every test we have),
- creating a tab and loading a test document in it,
- actually running the test (the setupTest function). In this case, you'll see it opens the inspector, starts the picker, then switches to another tab, then asserts that the picker is stopped.
- finishing up the test by removing the tab and calling finish().

Running the test goes like: ./mach mochitest-browser browser/devtools/inspector/test/browser_inspector_bug_962478_picker_stops_on_destroy.js

Note that bug 952277 landed 2 days ago, and with it came a refactor of the highlighter code in Toolbox.js which impacts your changes to that file. Basically, the startPicker and stopPicker functions have moved to another class. Other than that, your changes should still apply in these functions.

Comment 20

[Patrick Brosset <:pbro>]

Attachment: bug962478-stop-inspector-on-toolbox-destroy v1.patch

Taking the bug over as there's been no activity and it would be really good to get it uplifted to Aurora, since this can be pretty frustrating.

Mike, the bug contains:
- changes to the toolbox's highlighterutils so that startPicker and stopPicker always return promises, to make the callers code simpler
- changes to the toolbox's destroyInspector method
  - it now calls stopPicker, which is the actual fix for this bug
  - refactored the nested promises a bit with Task.spawn
- changes to the toolbox's destroy method: making sure that UI buttons are removed after the inspector is destroyed
- added a new test: browser_inspector_bug_962478_picker_stops_on_destroy.js
- simplified another (somehow related) test: browser_inspector_bug_961771_picker_stops_on_tool_select.js that had useless code and imports.

Comment 21

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #20)
> Mike, the bug contains:
Sorry, I meant: the *patch contains ...

Also: ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=18ed5950565e

Comment 22

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8370613 [details] [diff] [review]
bug962478-stop-inspector-on-toolbox-destroy v1.patch

Review of attachment 8370613 [details] [diff] [review]:
-----------------------------------------------------------------

r+ if the following issue is addressed.

::: browser/devtools/inspector/test/browser_inspector_bug_961771_picker_stops_on_tool_select.js
@@ +24,5 @@
>        Task.spawn(function() {
>          yield toolbox.highlighterUtils.startPicker();
>          yield toolbox.selectNextTool();
> +        yield pickerStopped;
> +        ok(true, "picker-stopped event fired after switch tools, so picker is closed");

If the pickerStopped event is not fired then what happens? It would be great if the test would fail and we knew why. I am guessing that at the moment it will just timeout.

At the very least we need to add info("Checking picker stopped event");

Comment 23

[Patrick Brosset <:pbro>]

Attachment: bug962478-stop-inspector-on-toolbox-destroy v2.patch

(In reply to Michael Ratcliffe - PTO until 28 JAN [:miker] [:mratcliffe] from comment #22)
> Comment on attachment 8370613 [details] [diff] [review]
> bug962478-stop-inspector-on-toolbox-destroy v1.patch
> 
> Review of attachment 8370613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ if the following issue is addressed.
> 
> :::
> browser/devtools/inspector/test/
> browser_inspector_bug_961771_picker_stops_on_tool_select.js
> @@ +24,5 @@
> >        Task.spawn(function() {
> >          yield toolbox.highlighterUtils.startPicker();
> >          yield toolbox.selectNextTool();
> > +        yield pickerStopped;
> > +        ok(true, "picker-stopped event fired after switch tools, so picker is closed");
> 
> If the pickerStopped event is not fired then what happens? It would be great
> if the test would fail and we knew why. I am guessing that at the moment it
> will just timeout.
> 
> At the very least we need to add info("Checking picker stopped event");
Thanks Mike for the review.
As discussed, there's no real way here to check that the event wasn't called. 
So I added a bunch of info() statements to make test debugging easier.

Also, I rebased the patch with an incoming change made to the Toolbox in bug 966970 which also touched the destroyInspector function.

It's the same patch otherwise.

And try is green.

Comment 24

[Patrick Brosset <:pbro>]

Fixed in fx-team https://hg.mozilla.org/integration/fx-team/rev/0f1bc0a9caa4

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f1bc0a9caa4

Comment 26

[Patrick Brosset <:pbro>]

Comment on attachment 8370946 [details] [diff] [review]
bug962478-stop-inspector-on-toolbox-destroy v2.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): a regression introduced by bug 916443
User impact if declined: no click/mouse-move events will go through to the content page if the steps described in comment 0 are executed.
Testing completed (on m-c, etc.): Yes, tests added, try build green.
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch: None

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Doesn't apply cleanly to Aurora. Please provide a branch patch or request approval for whatever this depends on.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

This depends on bug 966970. Seems easiest to just request Aurora approval on that as well.

Comment 29

[Patrick Brosset <:pbro>]

That's right, it depends on bug 966970. I've tried applied bug 966970's patch on aurora followed by this one, and they applied cleanly.
Going to request approval for 966970.
Thanks.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/14dcd01a2b01

Comment 31

[Bogdan Maris, Desktop QA]

Reproduced the initial issue on older nightly build and verified that the issue is fixed using latest Aurora on Windows 7 64bit, Mac OS X 10.9.1 and Ubuntu 12.04 32bit.

Comment 32

[Paul Silaghi, QA [:pauly]]

Verified fixed 30.0a2 (2014-03-18), win 7 x64