294815 - Possible to create a <browser> with no securityUI by accident

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Say an extension overlays in a <script> that accesses the tabbrowser after it's
in the DOM, but before frames have been constructed.  That will cause XBL
binding attachment, but this.docShell will be null in browser.xml, since the
frame owns that.

mconnor and I think that we should try attaching securityUI any time the getter
is called, our member is null, and our attribute is set appropriately.

Comment 1

[chris hofmann]

mconnor says patch coming up..

Comment 2

[Mike Connor [:mconnor]]

Attachment: move init call into the getter

instead of initing in the constructor, move the init to the getter.  This can
still throw, but that's valuable.  When this gets called from chrome we should
have a docShell then and not fail.

Comment 3

[[e-mail]]

Attachment: Update Suite's workaround

The Suite's workaround had one error since jag wrote it, and a second error
since bryner wrote bfcache; this should fix both. The code is in the style of
the session history fix but I couldn't be bothered to quote that in the diff.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 184049 [details] [diff] [review]
Update Suite's workaround

I guess this is ok (so sr+), but I'd really prefer fixing this in the browser
binding a la the toolkit fix...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 184045 [details] [diff] [review]
move init call into the getter

r=pseudo-toolkit-peer

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 184045 [details] [diff] [review]
move init call into the getter

a=me for 1.8b2.

/be

Comment 7

[Mike Connor [:mconnor]]

toolkit patch checked in, just xpfe left.

Comment 8

[[e-mail]]

(In reply to comment #4)
>I'd really prefer fixing this in the browser binding a la the toolkit fix...
But you still have to remember to call the securityUI getter.

The other approach would be to init session history and security in the browser
binding's loadURIWithFlags method instead of in the constructor.

Comment 9

[[e-mail]]

Comment on attachment 184049 [details] [diff] [review]
Update Suite's workaround

Oops, an extra } crept in on the line before the catch. Sorry about that :-[

Comment 10

[Steffen Wilberg]

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

Comment 11

[jag (Peter Annema)]

It'd be nice if we could fix this in the browser binding, but I don't like the
solution of moving it from browser construction to property access. This means
that anyone using the browser will have to access securityUI explicitely (even
if it's just a dummy call) to make sure securityUI is properly initialized, and
they'll have to make sure that is done early enough to have the securityUI
object pick up all the notifications it's interested in.

Is there a way for us to find out when a frame or the docshell is constructed
and hook up both session history and security UI from there?

Comment 12

[Boris Zbarsky [:bzbarsky]]

The constructor could access securityUI to make sure we try to make it once
(mconnor, does toolkit need to do that?  Seems like it does).

> Is there a way for us to find out when a frame or the docshell is constructed

At the moment, no.  I plan to move ownership of the docshell into the binding in
1.9, at which point this should all get simpler.

Comment 13

[jag (Peter Annema)]

Ah, I'll be looking forward to 1.9 then :-)

To address another aspect of this, could we say "hey, there's a point before
which you simply can't and shouldn't try to access {tab}browser"?

Comment 14

[Boris Zbarsky [:bzbarsky]]

We can say, sure.  How to enforce, though, with extensions?  :(

Comment 15

[PikeUK]

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

Comment 16

[jag (Peter Annema)]

It wasn't so much about enforcement as awareness. Users installing extensions
always run the risk of screwing up their Mozilla, there's nothing new there.
It'd be nice if we could somewhere tell extension writers about don'ts so they
can find work-arounds (like doing whatever they need to do with tabbrowser or
browser from an onload or onbindingattached handler).

Comment 17

[Boris Zbarsky [:bzbarsky]]

Looks like Neil was right... mconnor's fix caused bug 295121 for Firefox.

Comment 18

[Gummi]

So, is the solution to attach an event handler to a load event (eg the
tabbrowser's load event)? I have this problem with deerpark - 1.8b2.
Re #14: I don't mind fixing my extension for now ;)

Comment 19

[Robert Kaiser]

seaMonkey is now using toolkit's browser.xml, is there anything we still need landed here or can we close this issue?

Comment 20

[[e-mail]]

(In reply to comment #19)
>SeaMonkey is now using toolkit's browser.xml, is there anything we still need
>landed here or can we close this issue?
No, there's nothing left to do here, unless jag wants to try to convince mconnor not to do the init in the getter ;-)

Comment 21

[Serge Gautherie (:sgautherie)]

Neil, can you obsolete your patch if(/as) it's not needed anymore ?

Comment 22

[jag (Peter Annema)]

I'm not entirely happy with doing this from the getter and doing a dummy get, but it'll have to do until ownership of the docshell has moved into the binding, per comment 12.

Marking this as fixed.