408668 - Browser should check if it is in the root docshell before wiring up session history

Status: REOPENED

(Toolkit :: UI Widgets, defect)

Description

[Adam Kowalczyk]

Attachment: testcase (requires chrome privileges)

Browser's constructor generates the following exception:

Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.sessionHistory]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/bindings/browser.xml ::  :: line 582"  data: no]
Source File: chrome://global/content/bindings/browser.xml
Line: 588

Doesn't happen with Firefox 2.

Comment 1

[Adam Kowalczyk]

It turns out that you can only wire up session history in the root docshell and you should use "disablehistory" attribute with browsers in other docshells. This bug was exposed by bug 353403 - previously we used to swallow the exception.

Nickolay, I just wanted to confirm that the above reasoning correct, so I can document it on MDC.

Comment 2

[Adam Kowalczyk]

Come to think of it, if we know that session history can *never* be set up in non-top windows, wouldn't it make sense to check this automatically instead of forcing consumers to set disablehistory?

Comment 3

[Nickolay_Ponomarev]

(In reply to comment #1)
> It turns out that you can only wire up session history in the root docshell and
> you should use "disablehistory" attribute with browsers in other docshells.

I don't remember much about this, but I think session history is available for <browser type="content"> and the like. Neil should know more.

Comment 4

[[e-mail]]

(In reply to comment #2)
>Come to think of it, if we know that session history can *never* be set up in
>non-top windows, wouldn't it make sense to check this automatically instead of
>forcing consumers to set disablehistory?
Shouldn't they be using iframes instead?

(In reply to comment #3)
>(In reply to comment #1)
>>It turns out that you can only wire up session history in the root docshell and
>>you should use "disablehistory" attribute with browsers in other docshells.
>I don't remember much about this, but I think session history is available for
><browser type="content"> and the like. Neil should know more.
Yes, you can only use history on content browser types.

Comment 5

[Adam Kowalczyk]

(In reply to comment #3)
> I don't remember much about this, but I think session history is available for
> <browser type="content"> and the like. Neil should know more.

It's limited to root docshell, too:
http://mxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#3407 

(In reply to comment #4)
> Shouldn't they be using iframes instead?

In case of my extension I am loading an untrusted page, so I definitely need a browser, is that right? Also, browser has plenty of additional properties and methods that a consumer might need. iframe doesn't always cut it.

I am reopening and morphing the bug.

Comment 6

[Adam Kowalczyk]

See also bug 412171 comment 19.

Comment 7

[Serge Gautherie (:sgautherie)]

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

Now that bug 406697 (exceptions) is fixed,
I get one of this on the first opening of the sidebar (after each application start):
{{
Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.sessionHistory]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/bindings/browser.xml ::  :: line 644"  data: no]
Source File: chrome://global/content/bindings/browser.xml
Line: 650
}}

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> Now that bug 406697 (exceptions) is fixed,

More like s/exceptions/strict warning/ actually.

Comment 9

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

(In reply to comment #5)
> In case of my extension I am loading an untrusted page, so I definitely need a
> browser, is that right?

The XPCNativeWrapper documentation implies that that XUL iframes with type="content" are also untrusted <http://developer.mozilla.org/en/docs/XPCNativeWrapper#What_is_a_trusted_window.3F>.

Comment 10

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

Attachment: testcase for <iframe type="content">

(In reply to comment #9)
> (In reply to comment #5)
> > In case of my extension I am loading an untrusted page, so I definitely need a
> > browser, is that right?
> 
> The XPCNativeWrapper documentation implies that that XUL iframes with
> type="content" are also untrusted
> <http://developer.mozilla.org/en/docs/XPCNativeWrapper#What_is_a_trusted_window.3F>.

... which is confirmed by this testcase when loaded as chrome (when loaded as content it behaves differently, not sure if that's a bug or not).

Comment 11

[Adam Kowalczyk]

Thanks for clarifying. Are you therefore suggesting that we shouldn't make this change (i.e. make it easier to use browsers in non-root documents)?

Comment 12

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

(In reply to comment #11)
> Thanks for clarifying. Are you therefore suggesting that we shouldn't make this
> change (i.e. make it easier to use browsers in non-root documents)?

I think we definitely should make this change, since the current behavior is nonsensical.  I was just pointing out that it is possible to use an iframe to load untrusted content (and wanting to verify it for myself as well, since I do it).

Of course, if you need the other machinery browser widgets provide, then it still makes sense to use them, and in any case they shouldn't require you to specify attributes on threat of exception.

Comment 13

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

> (when loaded as content it behaves differently, not sure if that's a bug or
> not).

According to dveditz, this is expected behavior, since the remote XUL file is content, and data: URLs loaded by content are considered to come from the same security context.

Comment 14

[[e-mail]]

(In reply to comment #10)
>when loaded as content it behaves differently, not sure if that's a bug or not
type= is ignored for content frames; there's no "onion" effect here.

Comment 15

[Damon Sicore (:damons)]

Not blocking on this based on Comment #13.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> I get one of this on the first opening of the sidebar (after each application
> start):

That was bug 412171.

*****

Bug 412171 comment 22:
{{
Mike Connor   2008-03-31 13:44:15 PDT

[...] 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 17

[Samuel Sidler (old account; do not CC)]

Not wanted, also based on comment 13.