579954 - sometimes gBrowser cannot be accessed

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[David Dahl :ddahl]

Sometimes when getting gBrowser it is null inside of the console code.

Comment 1

[David Dahl :ddahl]

Attachment: fix-gBrowser-accessor.diff

Comment 2

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

Not sure what the proper tookit way to do this is, but those QIs seem a little odd. Is it possible that the creator of the HUDservice can set the gBrowser variable from the outside via a setGBrowser() method in the service? That'd seem less error prone and the instance could hold onto it for the duration of the console's existence.

Comment 3

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

Comment on attachment 458388 [details] [diff] [review]
fix-gBrowser-accessor.diff

this looks ok to me, but need mossop's stamp.

Comment 4

[Dave Townsend [:mossop]]

Can you explain why this problem occurs and why this fix works. Why is wrappedJSObject necessary? Can this be tested?

Comment 5

[David Dahl :ddahl]

(In reply to comment #4)
> Can you explain why this problem occurs and why this fix works. Why is
> wrappedJSObject necessary? Can this be tested?

I cannot explain it, it has been a problem for a while, When I first noticed it I hacked this together (by looking at the tabBrowser.xml code). Why this works, and the xulWindow.gBrowser does not is strange.

If I do not use wrappedJSObject, I get null. Perhaps another way to fix this is to check if the xulWindow is a browser window at all. I keep forgetting to file a bug about making sure the window that is handed to this method is a chrome browser window and not an alert or other type of window.

Comment 6

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

Comment on attachment 458388 [details] [diff] [review]
fix-gBrowser-accessor.diff

We shouldn't be introducing calls to getElementById("content"). I see no reason why the getter would ever fail here, so if it is we need to understand why. xulWindow not being a browser window could do it, but in that case I wouldn't expect the getElementById to succeed either (unless this is a view-source window, maybe).

Comment 7

[David Dahl :ddahl]

Looks like I already have code for getting gBrowser in the HUDService:

Services.wm.getMostRecentWindow("navigator:browser").gBrowser;


Perhaps I can just replace all of the "expected" gBrowser references with that?

Comment 9

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

nsIBrowserGlue::getMostRecentBrowserWindow is probably a better choice than getMostRecentWindow("navigator:browser") (though note that it does not return popup windows).

Comment 10

[David Dahl :ddahl]

Attachment: Fix for gBrowser is undefined

I think this approach is cleaner, no more using getElementById("content"), getting the gBrowser via Services.wm

Comment 11

[David Dahl :ddahl]

(In reply to comment #9)
> nsIBrowserGlue::getMostRecentBrowserWindow is probably a better choice than
> getMostRecentWindow("navigator:browser") (though note that it does not return
> popup windows).

Is there a foolproof way to get the most recent window (that should be debuggable) regardless?

Comment 12

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

I don't really understand the question. Is getMostRecentBrowserWindow not suitable because it doesn't include popups? I guess you should just duplicate it's implementation with that tweak, if so...

But that raises the question of why you need to find the "most recent" window anyways. getMostRecentWindow should be avoided if at all possible - getting the window directly from e.g. the invoked UI/event/whatever is going to be more reliable. I'm not really familiar with the HUDService code, though, so maybe I'm missing something?

Comment 13

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

Comment on attachment 459171 [details] [diff] [review]
Fix for gBrowser is undefined

This needs to be reviewed before it can get approval, right?

Comment 14

[David Dahl :ddahl]

(In reply to comment #13)
> Comment on attachment 459171 [details] [diff] [review]
> Fix for gBrowser is undefined
> 
> This needs to be reviewed before it can get approval, right?

Johnath was telling us this morning that getting blocking or approval early might be the way to go right now.

Comment 15

[David Dahl :ddahl]

(In reply to comment #12)
> I don't really understand the question. Is getMostRecentBrowserWindow not
> suitable because it doesn't include popups? 

I thought not, as you might want to debug popups, but now that I think of it, i have not tested that yet, etc... 
.
> 
> But that raises the question of why you need to find the "most recent" window
> anyways. getMostRecentWindow should be avoided if at all possible - getting the
> window directly from e.g. the invoked UI/event/whatever is going to be more
> reliable. I'm not really familiar with the HUDService code, though, so maybe
> I'm missing something?

I don't think you are missing anything.:)

Ok, now that makes me think the event option, etc... might be better.

Comment 16

[Dave Townsend [:mossop]]

(In reply to comment #14)
> (In reply to comment #13)
> > Comment on attachment 459171 [details] [diff] [review] [details]
> > Fix for gBrowser is undefined
> > 
> > This needs to be reviewed before it can get approval, right?
> 
> Johnath was telling us this morning that getting blocking or approval early
> might be the way to go right now.

As a rule you can only request approval after a patch has been reviewed.

Comment 17

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

(In reply to comment #15)
> (In reply to comment #12)
[...]
> > But that raises the question of why you need to find the "most recent" window
> > anyways. getMostRecentWindow should be avoided if at all possible - getting the
> > window directly from e.g. the invoked UI/event/whatever is going to be more
> > reliable. I'm not really familiar with the HUDService code, though, so maybe
> > I'm missing something?
> 
> I don't think you are missing anything.:)
> 
> Ok, now that makes me think the event option, etc... might be better.

that is what I was getting at in comment #2.

Comment 18

[Johnathan Nightingale [:johnath]]

Should the patch here be up for review?

What's the impact of this bug for the end user? Can't make a blocking decision without that, and comment 0 is ambiguous on the point.

Comment 20

[(no longer active)]

(In reply to comment #18)
> What's the impact of this bug for the end user? Can't make a blocking decision
> without that, and comment 0 is ambiguous on the point.

What I saw in bug 580652 for example was that the console was completely broken.  So I think this bug deserves to be a blocker.

Comment 21

[Johnathan Nightingale [:johnath]]

(In reply to comment #20)
> (In reply to comment #18)
> > What's the impact of this bug for the end user? Can't make a blocking decision
> > without that, and comment 0 is ambiguous on the point.
> 
> What I saw in bug 580652 for example was that the console was completely
> broken.  So I think this bug deserves to be a blocker.

Yep. Then this needs to get fixed immediately. beta3+ - David - can you get this in this week (if not today?) (Once reviewed, of course)

Comment 22

[David Dahl :ddahl]

(In reply to comment #21)

> Yep. Then this needs to get fixed immediately. beta3+ - David - can you get
> this in this week (if not today?) (Once reviewed, of course)

Yes indeedy. (I was on PTO yesterday.) This is prioritized ahead of all reviews.

Comment 23

[David Dahl :ddahl]

(In reply to comment #17)

> > Ok, now that makes me think the event option, etc... might be better.
> 
> that is what I was getting at in comment #2.

Does the approach in comment #2 fall down when you have a new chrome window with a new tabbrowser? DUMB question: is the tabbrowser a singleton among all windows?

Comment 24

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

There is one tabbrowser per window, and it never changes.

Comment 25

[David Dahl :ddahl]

Attachment: v3 Fix for gBrowser is undefined

running a build here so could not test this patch yet

Comment 26

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

So is the plan to get rid of currentContext in a followup?

Comment 27

[David Dahl :ddahl]

(In reply to comment #2)
> Not sure what the proper tookit way to do this is, but those QIs seem a little
> odd. Is it possible that the creator of the HUDservice can set the gBrowser
> variable from the outside via a setGBrowser() method in the service? That'd
> seem less error prone and the instance could hold onto it for the duration of
> the console's existence.

I think the problem with this approach is there are potentially many gBrowser instances, and you don't want the accounting task of keeping track of gBrowsers in the service as well. It seems like this "lazier" approach might be better.

Comment 28

[David Dahl :ddahl]

(In reply to comment #26)
> So is the plan to get rid of currentContext in a followup?

Well, I use that function to toggle the HUD on and off as well as to get the gBrowser that is associated with the last window used. I would like to know what you think is a better way to do this. If you can outline a better approach, I will the file the bug and get it moving.

Comment 29

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

Comment on attachment 460597 [details] [diff] [review]
v3 Fix for gBrowser is undefined

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

>   windowInitializer: function HS_WindowInitalizer(aContentWindow)

I was pretty surprised to discover that this code is running on each window creation. It's going to have a very negative effect on Tp, so we need to solve this in bug 568629 for sure. There appear to be other issues with this as well - I have no idea how it doesn't result in hundreds of TabClose listeners being added without ever being removed, for example.

>-    var xulWindow = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>-      .getInterface(Ci.nsIWebNavigation)
>-      .QueryInterface(Ci.nsIDocShellTreeItem)
>-      .rootTreeItem
>-      .QueryInterface(Ci.nsIInterfaceRequestor)
>-      .getInterface(Ci.nsIDOMWindow);

Let's replace this with:

var xulWindow = aContentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                              .getInterface(Ci.nsIWebNavigation)
                              .QueryInterface(Ci.nsIDocShell)
                              .chromeEventHandler.ownerDocument.defaultView;

and then do the check for gBrowser on that.

>   getCurrentContext: function FAH_getCurrentContext()

I'd like to get rid of this function. I don't fully understand the ownership models of the various objects involved, though. I assume there is only ever a single HUDService instance, and it appears that it keeps track of individual content windows. It's easy (though not necessarily cheap) to map content windows to their chrome window parent (as the snippet of code above does), so there should be no need to ever get the "most recent window".

Comment 30

[David Dahl :ddahl]

(In reply to comment #29)

> >   getCurrentContext: function FAH_getCurrentContext()
> 
> I'd like to get rid of this function. I don't fully understand the ownership
> models of the various objects involved, though. I assume there is only ever a
> single HUDService instance, and it appears that it keeps track of individual
> content windows. It's easy (though not necessarily cheap) to map content
> windows to their chrome window parent (as the snippet of code above does), so
> there should be no need to ever get the "most recent window".

Can I file a followup bug for that? This will impact the creation and destruction of each web console UI.

Comment 31

[David Dahl :ddahl]

Attachment: v4 Fix for gBrowser is undefined

This patch also checks to make sure gBrowser.tabContainer exists. Looks like view-source windows and other windows like the error console triggered this bug.

Comment 32

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

Yes, a followup is fine!

Comment 33

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

Comment on attachment 461368 [details] [diff] [review]
v4 Fix for gBrowser is undefined

>diff --git a/testing/mochitest/MochiKit/MochiKit.js b/testing/mochitest/MochiKit/MochiKit.js

This being included is a mistake, I'm assuming.

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

>   windowInitializer: function HS_WindowInitalizer(aContentWindow)

>+    let gBrowser = xulWindow.gBrowser;
>+
>+    if (!gBrowser) {
>+      // do not do anything unless we have a gBrowser/Firefox (navigator) window
>+      return;
>+    }
>+
>+    if (!gBrowser.tabContainer) {
>+      // also do not do anything unless we have a tabContainer. This may be a
>+      // view-source window or other type of non-browser window.
>+      return;
>+    }

Kind of unfortunate that the view source window has its own gBrowser that isn't a tabbrowser... Let's just make this:

var docElem = xulWindow.document.documentElement;
if (!docElem || docElem.getAttribute("windowtype") != "navigator:browser" || !xulWindow.gBrowser)
  return;

r=me with that change.

Comment 34

[David Dahl :ddahl]

(In reply to comment #33)
> Comment on attachment 461368 [details] [diff] [review]
> v4 Fix for gBrowser is undefined
> 
> >diff --git a/testing/mochitest/MochiKit/MochiKit.js b/testing/mochitest/MochiKit/MochiKit.js
> 
> This being included is a mistake, I'm assuming.

yes.

sometimes hg reports changed files that have no changes. so strange.

Comment 35

[David Dahl :ddahl]

Attachment: v5 Fix for gBrowser is undefined

Changed as per Gavin's comments

Comment 36

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

need to fix the comment to remove reference to "tabContainer" (something along the lines of "isn't a browser window" would be fine)

Comment 37

[David Dahl :ddahl]

Attachment: [checked-in] v5.1 Fix for gBrowser is undefined

changed the comment to:

      // Do not do anything unless we have a browser window.
      // This may be a view-source window or other type of non-browser window.

Comment 38

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

Comment on attachment 461393 [details] [diff] [review]
[checked-in] v5.1 Fix for gBrowser is undefined

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