283116 - browser.js Shutdown can have exception in removeObserver

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the analysis in bug 283088 was at least partly incorrect.

In browser.js, the following two addObserver and removeObserver calls are
supposedly balanced:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.382&mark=691-692#686
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.382&mark=905#900

However, they're only really balanced if the last tab closed in a window is the
one that was there when it was opened.

I made the mistake of adding the second of these, which breaks the whole rest of
the Shutdown function.

If the addObserver call cited above is actually still needed, I'm really not
sure what needs to be done.  However, I'm hoping that it's not still needed. 
I'll look into it.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Here's an idea:  let's not make the first browser window different from all the
rest and have the disablehistory="true" attribute, at let's not try to
duplicate everything browser.xml would do without that disablehistory="true".

I haven't found any regressions from this yet, but I've only been running it
briefly.  It seems logical.  If I do catch something, though, there's always
the workaround of changing the disablehistory attribute on that one browser
after startup...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Does the seamonkey tabbrowser have code like that too?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The equivalent seamonkey tabbrowser code is even messier and used to have
references to two bugs (bug 113076 and bug 139522) the second of which has
security implications.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

...but that code doesn't have the problematic browser:purge-session-history
observer.

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 175099 [details] [diff] [review]
patch

I don't think this should break anything, but we should have lots of time to
pick up regressions if they happen.  

I do agree that they shouldn't based on this, as cvs blame puts this as a perf
boost fix for blank homepages.	I don't know if tinderboxen have about:blank as
homepages, but I doubt this should affect startup that much, especially as
compared to the downside on the other end.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> I do agree that they shouldn't based on this, as cvs blame puts this as a perf
> boost fix for blank homepages.

Ah, I didn't see that in cvs blame anywhere.  That actually does sound like a
valid reason (esp. if it prevents history from being loaded until after we've
shown the browser window).  I'll look into this a bit more (since the
alternative is quite simple as well).

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

It actually does seem to make a startup order difference (and not just for
about:blank as a homepage).  (I was looking at whether global history was
created before or after the first paint, and that creation probably involves
reading in the global history database.)

So this makes the code a little clearer (adds a comment and moves the
addObserver to along with the rest of the code), and adds the fix of removing
the disablehistory attribute dynamically.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 175181 [details] [diff] [review]
patch

there's something wrong here...

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 175181 [details] [diff] [review]
patch

r=me, just add a blank line before the comment about the disablehistory
attribute.

This is definitely a lot better, and given some of the mork perf issues with
loading global history with huge history files, it probably would make a bigger
impact than I was expecting.

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 175181 [details] [diff] [review]
patch

oops!

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

I forgot to remove the removeObserver call.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 265906 has been marked as a duplicate of this bug. ***

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 175186 [details] [diff] [review]
patch

ok.  same nit about the line before the disablehistory comment, mostly style
consistency

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2005-02-22 17:35 -0800.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that this was fixing a mistake I made in bug 239833.