597103 - HUDService.deactivateHUDForContext(tab) fails when the given tab is not from the focused window

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Mihai Sucan [:msucan]]

If one invokes HUDService.deactivateHUDForContext(tab) when tab is not from the currently focused window, the method throws an error. The problem is caused because the code always checks the currentContext() - the currently focused window. The fix is really trivial - writing the mochitest takes more lines. :)

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed fix and test code

Proposed fix and test code.

Comment 2

[Mihai Sucan [:msucan]]

Attachment: updated patch

Rebased and updated the patch with some test code fixes.

Comment 3

[Mihai Sucan [:msucan]]

Attachment: updated patch

Fixed a bug which caused failures in the upcoming patch for bug 594523.

Please note that I also commented HUDService.shutdown() in the big tests file, since it causes intermittent failures on my system. Sometimes the executeSoon() function executes before finish() which makes the Web Console unusable - subsequent tests fail.

Comment 4

[David Dahl :ddahl]

Comment on attachment 477231 [details] [diff] [review]
updated patch

looks good

Comment 5

[Mihai Sucan [:msucan]]

Comment on attachment 477231 [details] [diff] [review]
updated patch

Thanks David for the feedback+!

Asking for review and approval2.0. This patch fixes an important bug in the way we deactivate the Web Console from each tab. This bug is also a dependency for bug 594523. Thanks!

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 477231 [details] [diff] [review]
updated patch

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>   deactivateHUDForContext: function HS_deactivateHUDForContext(aContext)

>+    let window = aContext.linkedBrowser.contentWindow;
>+    let hudId = "hud_" + aContext.linkedPanel;
>+    let displayNode = aContext.ownerDocument.getElementById(hudId);

This spreads an existing problem with HUD code that I noticed in bug 596371 (see bug 596371 comment 10 and bug 596371 comment 15). You shouldn't rely on getElementById() returning the anonymous notificationbox element. I'll file a bug on the general problem, but in the mean time, we shouldn't make it worse. This should be able to use aContext.ownerDocument.defaultView.getNotificationBox(aContext.linkedBrowser.contentWindow) instead.

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js b/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js

> function testEnd() {
>   // testUnregister();
>   executeSoon(function () {
>     HUDService.deactivateHUDForContext(tab);
>-    HUDService.shutdown();
>+    //HUDService.shutdown();
>   });

This bothers me a bit - why is the executeSoon needed at all? Assuming it really is needed, can't you just also put the finish() inside the executeSoon? And why is testUnregister() duplicating the executeSoon()ed code?

Comment 7

[Mihai Sucan [:msucan]]

(In reply to comment #6)
> Comment on attachment 477231 [details] [diff] [review]
> updated patch
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >   deactivateHUDForContext: function HS_deactivateHUDForContext(aContext)
> 
> >+    let window = aContext.linkedBrowser.contentWindow;
> >+    let hudId = "hud_" + aContext.linkedPanel;
> >+    let displayNode = aContext.ownerDocument.getElementById(hudId);
> 
> This spreads an existing problem with HUD code that I noticed in bug 596371
> (see bug 596371 comment 10 and bug 596371 comment 15). You shouldn't rely on
> getElementById() returning the anonymous notificationbox element. I'll file a
> bug on the general problem, but in the mean time, we shouldn't make it worse.
> This should be able to use
> aContext.ownerDocument.defaultView.getNotificationBox(aContext.linkedBrowser.contentWindow)
> instead.

I wasn't aware of that situation. Thanks for pointing it out to me.

Will fix the patch.

> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js b/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
> 
> > function testEnd() {
> >   // testUnregister();
> >   executeSoon(function () {
> >     HUDService.deactivateHUDForContext(tab);
> >-    HUDService.shutdown();
> >+    //HUDService.shutdown();
> >   });
> 
> This bothers me a bit - why is the executeSoon needed at all? Assuming it
> really is needed, can't you just also put the finish() inside the executeSoon?
> And why is testUnregister() duplicating the executeSoon()ed code?

Same thoughts here... I just made a minimal change to make the code run fine (see comment 3).

I'll update the patch to remove the commented lines, and will call finish() inside executeSoon(), after deactivateHUDForContext().

Thanks for your review!

Comment 8

[Mihai Sucan [:msucan]]

Attachment: updated patch

Patch updated based on review.

I should note that the code wasn't trying to retrieve the notificationbox. It is trying to reach the HUD node. Anyhow, I made the change to first get to the notificationbox using gBrowser.getNotificationBox(aContext.linkedBrowser), then with a querySelector() I retrieve the HUD node.

Hopefully the updates properly address your review comments. Thanks!

Comment 9

[Mihai Sucan [:msucan]]

Attachment: updated patch

As per IRC discussion: dropping the use of gBrowser for getNotificationBox().

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 477607 [details] [diff] [review]
updated patch

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>   /**
>-   * Activate a HeadsUpDisplay for the current window
>+   * Activate a HeadsUpDisplay for the given tab context.
>    *
>-   * @param nsIDOMWindow aContext
>+   * @param Element aContext the tab element.
>    * @returns void

Can we just change the parameter name to "aTab" and stop using "context" in all of the methods for which "context" is a tab? Or better yet, stop passing around tabs entirely and just have these methods take content windows (from which you can get tabs/browsers/displayNodes etc.). Followup bug, I suppose.

>   deactivateHUDForContext: function HS_deactivateHUDForContext(aContext)

>-    var displayNode = this.getHeadsUpDisplay(hudId);

We need to get rid of this method. Followup bug?
>+    let hudId = "hud_" + nBox.getAttribute("id");

nBox.id

>+    if (hudId in this.displayRegistry && displayNode) {
>+      this.unregisterActiveContext(hudId);
>+      this.unregisterDisplay(displayNode);
>+      if (window) {
>+        window.focus();

I don't think it's possible for tab.linkedBrowser.contentWindow to be null, so the check should be unnecessary.

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js

>+Cu.import("resource://gre/modules/HUDService.jsm");

Browser chrome tests shouldn't import things into the global scope. Another followup bug?

Comment 11

[Mihai Sucan [:msucan]]

(In reply to comment #10)
> Comment on attachment 477607 [details] [diff] [review]
> updated patch
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >   /**
> >-   * Activate a HeadsUpDisplay for the current window
> >+   * Activate a HeadsUpDisplay for the given tab context.
> >    *
> >-   * @param nsIDOMWindow aContext
> >+   * @param Element aContext the tab element.
> >    * @returns void
> 
> Can we just change the parameter name to "aTab" and stop using "context" in all
> of the methods for which "context" is a tab? Or better yet, stop passing around
> tabs entirely and just have these methods take content windows (from which you
> can get tabs/browsers/displayNodes etc.). Followup bug, I suppose.
>

Good point. As far as I know there's a bug report about something like this, already. It's about storing a reference to the browser element and inferring stuff from that. Would help with the case you are suggesting as well. (can't find the bug number, argh)


> >   deactivateHUDForContext: function HS_deactivateHUDForContext(aContext)
> 
> >-    var displayNode = this.getHeadsUpDisplay(hudId);
> 
> We need to get rid of this method. Followup bug?

Not sure about this. The getHeadsUpDisplay() method is still *very* much user all over the place. Removing this method would perhaps mean a refactor of the HUDService.


> >+    let hudId = "hud_" + nBox.getAttribute("id");
> 
> nBox.id

Hehe, will do.


> >+    if (hudId in this.displayRegistry && displayNode) {
> >+      this.unregisterActiveContext(hudId);
> >+      this.unregisterDisplay(displayNode);
> >+      if (window) {
> >+        window.focus();
> 
> I don't think it's possible for tab.linkedBrowser.contentWindow to be null, so
> the check should be unnecessary.

Will update.


> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_597103_deactivateHUDForContext_unfocused_window.js
> 
> >+Cu.import("resource://gre/modules/HUDService.jsm");
> 
> Browser chrome tests shouldn't import things into the global scope. Another
> followup bug?

Reported bug 602970.


Thanks for the review+!

Comment 12

[Mihai Sucan [:msucan]]

Attachment: patch update 5

Patch update 5, based on review.

Comment 13

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch. Only test file changes.

Comment 14

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/298556684133