Closed Bug 579954 Opened 9 years ago Closed 9 years ago

sometimes gBrowser cannot be accessed

Categories

(DevTools :: General, defect)

x86
All
defect
Not set

Tracking

(blocking2.0 beta3+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta3+

People

(Reporter: ddahl, Assigned: ddahl)

References

Details

(Whiteboard: [kd4b3])

Attachments

(1 file, 5 obsolete files)

Sometimes when getting gBrowser it is null inside of the console code.
Blocks: 529086
Attached patch fix-gBrowser-accessor.diff (obsolete) — Splinter Review
Assignee: nobody → ddahl
Attachment #458388 - Flags: review?
Attachment #458388 - Flags: review? → review?(rcampbell)
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.
Attachment #458388 - Flags: review?(dtownsend)
Status: NEW → ASSIGNED
blocking2.0: --- → ?
Comment on attachment 458388 [details] [diff] [review]
fix-gBrowser-accessor.diff

this looks ok to me, but need mossop's stamp.
Attachment #458388 - Flags: review?(dtownsend) → review+
Attachment #458388 - Flags: review?(rcampbell) → review?(dtownsend)
Can you explain why this problem occurs and why this fix works. Why is wrappedJSObject necessary? Can this be tested?
(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 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).
Attachment #458388 - Flags: review?(dtownsend) → review-
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?
Duplicate of this bug: 580652
nsIBrowserGlue::getMostRecentBrowserWindow is probably a better choice than getMostRecentWindow("navigator:browser") (though note that it does not return popup windows).
Attached patch Fix for gBrowser is undefined (obsolete) — Splinter Review
I think this approach is cleaner, no more using getElementById("content"), getting the gBrowser via Services.wm
Attachment #458388 - Attachment is obsolete: true
Attachment #459171 - Flags: approval2.0?
(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?
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 on attachment 459171 [details] [diff] [review]
Fix for gBrowser is undefined

This needs to be reviewed before it can get approval, right?
Attachment #459171 - Flags: approval2.0?
(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.
(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.
(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.
(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.
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.
Duplicate of this bug: 580652
(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.
(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)
blocking2.0: ? → beta3+
(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.
(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?
There is one tabbrowser per window, and it never changes.
Attached patch v3 Fix for gBrowser is undefined (obsolete) — Splinter Review
running a build here so could not test this patch yet
Attachment #459171 - Attachment is obsolete: true
Attachment #460597 - Flags: feedback?
Attachment #460597 - Flags: feedback? → feedback?(gavin.sharp)
Attachment #460597 - Flags: feedback?(gavin.sharp) → review?(gavin.sharp)
So is the plan to get rid of currentContext in a followup?
(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.
(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 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".
Attachment #460597 - Flags: review?(gavin.sharp) → review-
(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.
Attached patch v4 Fix for gBrowser is undefined (obsolete) — Splinter Review
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.
Attachment #460597 - Attachment is obsolete: true
Attachment #461368 - Flags: review?(gavin.sharp)
Yes, a followup is fine!
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.
Attachment #461368 - Flags: review?(gavin.sharp) → review+
(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.
Attached patch v5 Fix for gBrowser is undefined (obsolete) — Splinter Review
Changed as per Gavin's comments
Attachment #461368 - Attachment is obsolete: true
Attachment #461378 - Flags: review+
Keywords: checkin-needed
need to fix the comment to remove reference to "tabContainer" (something along the lines of "isn't a browser window" would be fine)
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.
Attachment #461378 - Attachment is obsolete: true
Attachment #461393 - Flags: review+
Comment on attachment 461393 [details] [diff] [review]
[checked-in] v5.1 Fix for gBrowser is undefined

http://hg.mozilla.org/mozilla-central/rev/b45b63b9283f
Attachment #461393 - Attachment description: v5.1 Fix for gBrowser is undefined → [checked-in] v5.1 Fix for gBrowser is undefined
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [kd4b3]
Duplicate of this bug: 576782
Keywords: checkin-needed
Duplicate of this bug: 580352
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.