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

RESOLVED FIXED

Status

()

RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: bzbarsky, Assigned: neil)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

13 years ago
mconnor says patch coming up..
Flags: blocking-aviary1.1+
Created attachment 184045 [details] [diff] [review]
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.
(Assignee)

Comment 3

13 years ago
Created attachment 184049 [details] [diff] [review]
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.
Attachment #184049 - Flags: superreview?(bzbarsky)
Attachment #184049 - Flags: review?(jag)
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...
Attachment #184049 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 184045 [details] [diff] [review]
move init call into the getter

r=pseudo-toolkit-peer
Attachment #184045 - Flags: review+
Comment on attachment 184045 [details] [diff] [review]
move init call into the getter

a=me for 1.8b2.

/be
Attachment #184045 - Flags: approval1.8b2+
toolkit patch checked in, just xpfe left.
(Assignee)

Comment 8

13 years ago
(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.
(Assignee)

Comment 9

13 years ago
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

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

Comment 11

13 years ago
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?
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

13 years ago
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"?
We can say, sure.  How to enforce, though, with extensions?  :(

Comment 15

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

Comment 16

13 years ago
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).
Looks like Neil was right... mconnor's fix caused bug 295121 for Firefox.

Updated

13 years ago
Flags: blocking-aviary1.1+

Comment 18

13 years ago
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 ;)
Blocks: 282179

Updated

12 years ago
Assignee: mconnor → neil

Comment 19

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

Comment 20

11 years ago
(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 ;-)
Neil, can you obsolete your patch if(/as) it's not needed anymore ?
(Assignee)

Updated

11 years ago
Attachment #184049 - Attachment is obsolete: true
Attachment #184049 - Flags: review?(jag)

Comment 22

11 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.