594523 - When tab location changes, the HUDService fails to clear/update state information

Status: RESOLVED DUPLICATE of bug 611789

(DevTools :: General, defect, P1)

Description

[Mihai Sucan [:msucan]]

STR (from a mochitest file):

1. open tab 1
a) load urlA
b) open the Web Console
c) load urlB

2. open tab 2
a) open urlA
b) open the Web Console

3. call:
let display1 = HUDService.getDisplayByURISpec(urlA);
let display2 = HUDService.getDisplayByURISpec(urlB);
isnot(display1, display2, "the display nodes must be different");

Expected result: test passes.
Actual result: test fails.

Problem at hand: the HUDService fails to acknowledge the user navigated to a different URL in tab1, for that HUD object, so the uriRegistry holds the hud panel ID of both tabs for urlA.

Will attach a mochitest file that shows the problem.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: mochitest case

This is a mochitest case which shows the error.

Comment 2

[Kevin Dangoor]

I just tried this. The user impact appears to be that the messages for Tab 2 appear in the console for Tab 1.

This looks like the kind of bug that someone could reasonably hit and be quite confused by. I'm scheduling this for us to work on in beta 7.

Also, I'm requesting blocking status for this, because this bug would be ugly.

Comment 4

[David Dahl :ddahl]

The tests for this bug also need to trigger "back" and "forward" so we can properly update the console when we observe back and forward events.

Now, this may become a moot point once the lazy console lands, as it ties itself to the window in a different way, via the outer window ID

Comment 5

[Mihai Sucan [:msucan]]

Attachment: proposed fix and test code

Proposed fix and test code.

This patch includes the mochitest I had attached when I reported the bug.

This patch depends on the patch from bug 597103.

Notes on some changes I did:

- bug 595350 introduced closeConsoleOnTab() which serves the same purpose as the older deactivateHUDForContext()

- bug 597103 fixes an issue with the handling of deactivation of the Web Console from unfocused windows.

- this patch needed a way to remove the nsIWebProgressListener from all tabs once the Web Console is no longer active in any tab. Hence I had to unify closeConsoleOnTab() and deactivateHUDForContext(). This meant I also needed the fixes from bug 597103.

Comment 6

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased patch.

Comment 7

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 478761 [details] [diff] [review]
rebased patch

+    let foundHuds = false;
+    for (let id in this.displayRegistry) {
+      foundHuds = true;
+      break;
+    }

You're just looking for non-emptiness here, right? How about something like,

let foundHuds = this.displayRegistry.keys().length > 0;

... instead? Multiple locations.

I'd even consider breaking that into a separate method for readability.


Rest of the code + test file look good, I think.

This code may not be necessary if David's lazyConsole stuff fixes this, but good to have and the test might be useful there anyway.

Comment 8

[Mihai Sucan [:msucan]]

(In reply to comment #7)
> Comment on attachment 478761 [details] [diff] [review]
> rebased patch
> 
> +    let foundHuds = false;
> +    for (let id in this.displayRegistry) {
> +      foundHuds = true;
> +      break;
> +    }
> 
> You're just looking for non-emptiness here, right? How about something like,
> 
> let foundHuds = this.displayRegistry.keys().length > 0;
> 
> ... instead? Multiple locations.
> 
> I'd even consider breaking that into a separate method for readability.

Sure, I can change that. Will update the patch.


> Rest of the code + test file look good, I think.
> 
> This code may not be necessary if David's lazyConsole stuff fixes this, but
> good to have and the test might be useful there anyway.

I'm not sure either. I haven't seen the latest patches from David's lazy console stuff.Somehow I think this patch is still needed, but with some rebasing. His work touches on the aspects of window.console, while this code touches on aspects of how the HUD Service manages HUDs. Time will tell. :)

Thanks for the feedback+!

Comment 9

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch based on feedback from Robert. Thanks!

Asking for review from Shawn now.

Comment 10

[Shawn Wilsher :sdwilsh]

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

I'm going to defer to a browser peer on this because you are adding and twiddling a property on gBrowser (and using methods on it, which I'm not familiar with).  However, noticed some things that I'll comment on...

>+  removeProgressListener: function HS_removeProgressListener(aGBrowser)
>+  {
>+    let foundHuds = Object.keys(this.displayRegistry).length > 0;
>+
>+    if (!foundHuds && aGBrowser._WebConsoleProgressListenerAttached) {
It's not clear to me why you do this if there are no HUDs found.  I think a comment explaining this could be handy.

>+  onLocationChange:
>+  function CPL_onLocationChange(aBrowser, aProgress, aRequest, aLocation)
usually we'd put whatever arguments don't fit on the first line on the second one lined up with the first argument

>+    let oldIndex = history.index-1;
spaces around the minus please

comment 4 also seems to not be addressed in the test

Comment 11

[Mihai Sucan [:msucan]]

Attachment: patch update 2

Updated patch. Addressing comment 4 and the comments from Shawn. Sorry I missed that comment.

I added testing of going back and forward. This semi-worked with the previous patch, but I had to make some improvements to better handle more cases of navigation in history.

Thanks Shawn!

Comment 12

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

Comment on attachment 480414 [details] [diff] [review]
patch update 2

I think we need to focus on getting rid of these "registries", rather than doing more work to ensure they are properly updated.

Comment 13

[David Dahl :ddahl]

(In reply to comment #12)
> Comment on attachment 480414 [details] [diff] [review]
> patch update 2
> 
> I think we need to focus on getting rid of these "registries", rather than
> doing more work to ensure they are properly updated.

Do you have any general approaches in mind? It is a shame we have all of them, but I was unsure how to track things otherwise.

Comment 14

[Mihai Sucan [:msucan]]

Gavin: what I believe we can do is.... remove the uriRegistry, _headsUpDisplay and some other registries. I'd like us to keep only a minimum of:

- hudReferences which maps hudIds to HeadsUpDisplay objects (like we have now)
- windowIds which maps outer window IDs to hudIds.

This would allow us to: find the HUD object based on its hudId, or based on the windowId. It would also allow us to find the HUD object based on the window object. These are pretty much all of the cases we need.

Does this sound fine to you? If yes, then such work should be done in bug 611789. In that patch we can also remove getHeadsUpDisplay() ... as needed for bug 592469.

Comment 15

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

Yeah, that sounds like a good plan.

Comment 16

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 17

[Kevin Dangoor]

mass change: filter mail on BLOCKERSETTING

Comment 18

[Mihai Sucan [:msucan]]

Submitted the patch for bug 611789, which provides a better solution to the issue reported here. I am setting bug 611789 as a dependency.

Comment 19

[Mihai Sucan [:msucan]]

This is now fixed by bug 611789.