412171 - In <browser.xml>, "Error: this.docShell is null", when the sidebar opens

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

(This is the error which bug 404236 was initially reported as.)


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/20070515
SeaMonkey/1.5a] (nightly) (W2Ksp4)

No bug.


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008011303 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

0. (In a browser window.)
1. Open the sidebar, (by pressing F9).
1r. Get 8 times the following error
[
Error: this.docShell is null
Source File: chrome://global/content/bindings/browser.xml
Line: 0
]


I guess these are all the |onget=""| property attributes in the file.

The common code/cause should be
<http://mxr.mozilla.org/seamonkey/source/toolkit/content/widgets/browser.xml>
[
 252       <field name="_docShell">null</field>
 253 
 254       <property name="docShell"
 255                 onget="return this._docShell || (this._docShell =
this.boxObject.QueryInterface(Components.interfaces.nsIContainerBoxObject).docShell);"
 256                 readonly="true"/>
]
The assignment code (on the right) is the one which used to be in /xpfe.


I thought this could be bug 406697, but it does not seem so:

*fix 1:
No effect,
as expected from initial report build date (which was before the fix for bug
385224).

*fix 2: 
(Not tested, as fix 1 has no effect on the current bug.)

*fix 3: 
No effect,
although |this._docShell| is not null when |this._docShell = null;| is
executed.
NB: What surprised me is that |onPageHide()| is executed [except the 1st time]
immediately before |onPageShow()| when the sidebar (panel) is being _shown_,
rather than when the sidebar (panel) is being _hidden_ !
I guess the sidebar itself is hidden, but its panel is still considered shown,
until "rebuilt" when the sidebar is shown again...
The errors show up (after |onPageHide()| and) _before_ |onPageShow()|.

Comment 1

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008011305 Minefield/3.0b3pre] (nightly) (W2Ksp4)

(only) 1 error when opening Sidebar, (either Ctrl+B or Ctrl+H), too.
But only the first time for each window. ("due" to bug 406697 ?)

Comment 2

[Serge Gautherie (:sgautherie)]

Could be related/duplicate of bug 313048 !?
(which I didn't find before because the error text had changed in the meantime :-/)

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Isn't this a duplicate of bug 410559?

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> Isn't this a duplicate of bug 410559?

No, the error here and the exception there are two different issues.
If you believe they have the same cause/fix, explain it further.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

bug 410559 problems occur when trying to access properties of browser.webNavigation, which is defined by a getter:

<property name="webNavigation"
  onget="return this.docShell.QueryInterface(Components.interfaces.nsIWebNavigation);"

so, "docShell" is being accessed in these cases too. If "docShell" is null, we'd expect the failures seen in bug 410559.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> so, "docShell" is being accessed in these cases too. If "docShell" is null,
> we'd expect the failures seen in bug 410559.

"If" ? Did you checked whether or not |docShell| is actually null at that time ?
(Please, follow up this thread in that bug.)

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> *** This bug has been marked as a duplicate of bug 410559 ***

You are dup'ing the current bug to bug 410559, which you duped to bug 406697.
This seems to contradict my test(s) in comment 0.
Please, explain !

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #8)
> You are dup'ing the current bug to bug 410559, which you duped to bug 406697.

Silly mistake

> This seems to contradict my test(s) in comment 0.
> Please, explain !

Ok, I yield. Fix 1 from bug 406697 does not fix this bug (no matter how many ways I try it), so I am un-duping this bug.

Comment 10

[Mike Schroepfer]

Moving to blocking list to make sure something bad isn't going on here...

Comment 11

[Boris Zbarsky [:bzbarsky]]

I seriously doubt this is a docshell issue.  Most likely this is either an XBL issue or an issue in the binding itself.

That said, is there at least a regression range here?

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> That said, is there at least a regression range here?

Here is what I found:

The error appears with
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/2006092604 Minefield/3.0a1] (nightly) (W2Ksp4)
which gives
<http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&sortby=Date&hours=2&date=explicit&mindate=2006-09-25+04&maxdate=2006-09-26+05&cvsroot=%2Fcvsroot>
Likely the 3 checkins related to disabling Places:
[
2006-09-25 22:21	vladimir%pobox.com 	more turning-off-places bustage fixes
2006-09-25 20:52	vladimir%pobox.com 	b=353571, disable Places on trunk; r+sr=mconnor
2006-09-25 17:05	sspitzer%mozilla.org 	fix for bug #341654: search bookmarks broken (on trunk) if you build with --disable places
r=vlad
]

Before, from
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060215 Firefox/1.6a1] (nightly, 2006-02-15-14-trunk) (W2Ksp4)
{
benjamin%smedbergs.us	2006-02-15 10:46	 	Bug 327188 - enable places by default, r=bryner
}
to
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/2006092504 Minefield/3.0a1] (nightly) (W2Ksp4)
the feature is missing, because of the broken Places.
(a gap of 7+ months :-()

Immediately before, I tried
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060215 Firefox/1.6a1] (nightly, 2006-02-15-07-trunk) (W2Ksp4)
which outputs
{
Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsITypeAheadFind.setDocShell]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/bindings/browser.xml :: onPageHide :: line 455"  data: no]
Source File: chrome://global/content/bindings/browser.xml
Line: 455
}
when closing the sidebar;
but no error when opening it.

***

I guess SeaMonkey picked up this error when it switched to using Toolkit.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm going to stand by my diagnosis: this isn't a docshell bug.

Comment 14

[Boris Zbarsky [:bzbarsky]]

re-setting blocking flag that got lost

Comment 15

[Reed Loden [:reed]]

Need blocking1.9+ again due to move. :(

Comment 16

[Damon Sicore (:damons)]

Who owns this?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

The error is different now that docshell is created already before showing the
sidebar. The problem is setting sessionhistory on non-top-level-chrome-type-docshell.
Is there any reason not to disable sessionhistory for sidebar?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: disable sessionhistory [Checkin: Comment 24]

Myk, you probably know the sidebar code a lot better than I do.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsDocShell.cpp&rev=1.894&mark=3443-3466#3441

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 311267 [details] [diff] [review]
disable sessionhistory
[Checkin: Comment 24]

>Index: browser/base/content/browser.xul

>-      <browser id="sidebar" flex="1" autoscroll="false"
>+      <browser id="sidebar" flex="1" autoscroll="false" disablehistory="true"

It seems to me that a XUL author shouldn't have to specify this attribute on threat of exception, especially given the lack of documentation on it (the XUL reference describes some different attribute in its place <http://developer.mozilla.org/en/docs/XUL:browser#a-disablehistory>).

Instead, if the attribute is absent, the browser widget should simply suppress the exception; it should only report it if the user explicitly specifies disablehistory="false" and the browser isn't the root tree item.

For that matter, it's unclear to me why its docshell isn't the root tree item in this case, which is why nsDocShell::SetSessionHistory fails in my debugging session <http://lxr.mozilla.org/mozilla/source/docshell/base/nsDocShell.cpp#3457>.

Nevertheless, this at least gets rid of the error, and it seems like a reasonable change, given that we don't need session history for the sidebar, and given how liberally the attribute is sprinkled throughout the code, f.e. on the tabbrowser just below this code <http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.xul#511>, so r=myk.

But I'm not authorized to review this code; you'll need review from a browser owner or peer like Mike Connor.  Requesting review from him.

Comment 20

[Adam Kowalczyk]

(In reply to comment #19)
> It seems to me that a XUL author shouldn't have to specify this attribute on
> threat of exception, especially given the lack of documentation on it (the XUL
> reference describes some different attribute in its place
> <http://developer.mozilla.org/en/docs/XUL:browser#a-disablehistory>).
> 
> Instead, if the attribute is absent, the browser widget should simply suppress
> the exception; it should only report it if the user explicitly specifies
> disablehistory="false" and the browser isn't the root tree item.

I thought the same thing when I encountered this exception some time ago and I filed bug 408668 about it.

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)
> The error is different now that docshell is created already before showing the
> sidebar.

Actually,
this error changed in FF: see bug 408668 comment 7;
but is still the same, as in comment 0, in SM.

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 311267 [details] [diff] [review]
disable sessionhistory
[Checkin: Comment 24]

This is okay for now, I think that its probably right that the exception handling isn't quite right, but this works for now and we can rethink the widget behaviour for next platform cycle.

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-SM) <sidebarOverlay.xul> [Checkin: Comment 28]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008033101 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Port from FF patch.

Each <browser> is responsible for 4 errors: 2 * 4 = 8.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Checked in the patch for FF. Since the original report was for SeaMonkey,
keeping this still open.

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)
> (In reply to comment #17)
> > The error is different now that docshell is created already before showing the
> > sidebar.
> 
> Actually,
> this error changed in FF: see bug 408668 comment 7;
> but is still the same, as in comment 0, in SM.

Would someone know which patch changed this ? ("security" bug, fixed by Boris !?)
As I would like to check if that was a FF only patch,
which SeaMonkey could want to get too ?
Thanks.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #25)
> Would someone know which patch changed this ? ("security" bug, fixed by Boris
> !?)
IIRC, it was the architectural change by me. xul:browser starts loading when
attached to document, not when set visible. That is gecko-wide change, so
I'm not sure why SM behaves here differently than FF. Something different in chrome, probably.

Comment 27

[Karsten Düsterloh]

Comment on attachment 312823 [details] [diff] [review]
(Bv1-SM) <sidebarOverlay.xul>
[Checkin: Comment 28]

Looks good enough for the hack it is. :-/

Comment 28

[Andrew Schultz]

(landed the SM patch)

Comment 29

[Serge Gautherie (:sgautherie)]

See bug 408668 about fixing the root cause of this issue.

Comment 30

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008040106 Minefield/3.0pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008040616 SeaMonkey/2.0a1pre] (SEA-WIN32-TBOX-trunk) (W2Ksp4)

V.Fixed