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

RESOLVED FIXED

Status

()

Firefox
Developer Tools
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: msucan, Assigned: msucan)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [patchclean:1012])

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

7 years ago
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. :)
(Assignee)

Comment 1

7 years ago
Created attachment 476339 [details] [diff] [review]
proposed fix and test code

Proposed fix and test code.
Assignee: nobody → mihai.sucan
Status: NEW → ASSIGNED
Attachment #476339 - Flags: feedback?(ddahl)
(Assignee)

Updated

7 years ago
Whiteboard: [patchclean:0917]

Updated

7 years ago
Blocks: 594225
(Assignee)

Comment 2

7 years ago
Created attachment 477168 [details] [diff] [review]
updated patch

Rebased and updated the patch with some test code fixes.
Attachment #476339 - Attachment is obsolete: true
Attachment #477168 - Flags: feedback?(ddahl)
Attachment #476339 - Flags: feedback?(ddahl)
(Assignee)

Updated

7 years ago
Whiteboard: [patchclean:0917] → [patchclean:0921]
(Assignee)

Comment 3

7 years ago
Created attachment 477231 [details] [diff] [review]
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.
Attachment #477168 - Attachment is obsolete: true
Attachment #477231 - Flags: feedback?(ddahl)
Attachment #477168 - Flags: feedback?(ddahl)
(Assignee)

Updated

7 years ago
Blocks: 594523

Comment 4

7 years ago
Comment on attachment 477231 [details] [diff] [review]
updated patch

looks good
Attachment #477231 - Flags: feedback?(ddahl) → feedback+
(Assignee)

Comment 5

7 years ago
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!
Attachment #477231 - Flags: review?(gavin.sharp)
Attachment #477231 - Flags: approval2.0?
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?
Attachment #477231 - Flags: review?(gavin.sharp)
Attachment #477231 - Flags: review-
Attachment #477231 - Flags: approval2.0?
(Assignee)

Comment 7

7 years ago
(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!
(Assignee)

Comment 8

7 years ago
Created attachment 477582 [details] [diff] [review]
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!
Attachment #477231 - Attachment is obsolete: true
Attachment #477582 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Whiteboard: [patchclean:0921] → [patchclean:0922]
(Assignee)

Comment 9

7 years ago
Created attachment 477607 [details] [diff] [review]
updated patch

As per IRC discussion: dropping the use of gBrowser for getNotificationBox().
Attachment #477582 - Attachment is obsolete: true
Attachment #477607 - Flags: review?(gavin.sharp)
Attachment #477582 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Blocks: 597200
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?
Attachment #477607 - Flags: review?(gavin.sharp) → review+
blocking2.0: --- → betaN+
(Assignee)

Comment 11

7 years ago
(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+!
(Assignee)

Comment 12

7 years ago
Created attachment 481917 [details] [diff] [review]
patch update 5

Patch update 5, based on review.
Attachment #477607 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Whiteboard: [patchclean:0922] → [patchclean:1008]
(Assignee)

Comment 13

7 years ago
Created attachment 482515 [details] [diff] [review]
rebased patch

Rebased patch. Only test file changes.
Attachment #481917 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Whiteboard: [patchclean:1008] → [patchclean:1012]

Comment 14

7 years ago
http://hg.mozilla.org/mozilla-central/rev/298556684133

Updated

7 years ago
Keywords: checkin-needed

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.