304388 - Add hostname to title bar of windows that don't have location bars

Status: RESOLVED FIXED

(Firefox :: Security, defect)

Description

[Jesse Ruderman]

https://bugzilla.mozilla.org/show_bug.cgi?id=22183#c238 was only checked in on
the branch.  In a security bug meeting on Tuesday, we decided to do the same
thing for Firefox 1.5 (and trunk?).  I'll leave bug 22183 open for a long-term
solution.

Comment 1

[Jesse Ruderman]

Attachment: update dveditz's patch to trunk

I hand-applied dveditz's patch
(https://bugzilla.mozilla.org/show_bug.cgi?id=22183#c238) to the trunk and
edited a few code comments.  I wasn't able to test the code in
nsContentTreeOwner::SetTitle because the code in tabbrowser always seems to be
what determines the window title in Firefox.  I left out the SeaMonkey-specific
part of the patch.

cvs diff -p
xpfe/appshell/src/nsContentTreeOwner.cpp 
content/html/document/src/nsHTMLDocument.cpp 
toolkit/content/widgets/tabbrowser.xml

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 192787 [details] [diff] [review]
update dveditz's patch to trunk

It'll take me some time to get to this...  not even sure how much.

Comment 3

[Jesse Ruderman]

How can I test nsContentTreeOwner::SetTitle?  Build Camino?

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 192787 [details] [diff] [review]
update dveditz's patch to trunk

I'm not really a good reviewer for appshell or content code, it should be a
peer in the appropriate module if at all possible.

Comment 5

[Gervase Markham [:gerv]]

<sigh> I wish we'd had time to do something better. Still, as long as we don't
see this as the long-term solution...

Gerv

Comment 6

[Boris Zbarsky [:bzbarsky]]

So is there a reason this is not patching both tabbrowsers?  Is there a reason
that embedding builds don't have the same problem... or do they?

That said, doesn't the nsContentTreeOwner code just run whenever you load a new
page in a non-tabbrowser setup?  Just remove the hookup of the DOMTitleChanged
listener in tabbrowser to exercise it visually, I think.  And file a followup
bug on the fact that we have this code in 3 different places?

Comment 7

[Jesse Ruderman]

> So is there a reason this is not patching both tabbrowsers?

I don't build Seamonkey and didn't realize I was expected to help maintain that
fork.  If you want me to build Seamonkey, I can.

I don't know the answers to your other questions.  dveditz (the original author
of the patch) might.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 192787 [details] [diff] [review]
update dveditz's patch to trunk

This needs at least a similar patch for xpfe and a check on whether
nsWebBrowser has similar issues, as well as testing of the nsContentTreeOwner
code.  Please rerequest review when those have happened.

Comment 9

[Asa Dotzler [:asa]]

we're gonna have to take what we did for 1.0  because we dont' have anything
better and time's run out. Dan, can you drive this in?

Comment 10

[Daniel Veditz [:dveditz]]

Attachment: including merged xpfe/global changes

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 197923 [details] [diff] [review]
including merged xpfe/global changes

r=myself, this is just a trunk merge of a previously reviewed patch (r=mconnor,
sr=bzbarsky, a=asa)

Still requesting sr=bz since he had a problem with the first version here.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 197923 [details] [diff] [review]
including merged xpfe/global changes

I guess this is ok given the situation, but could we please file a bug to have
one and only one place where this code lives by 1.9?

Comment 13

[Daniel Veditz [:dveditz]]

fixed on trunk

Comment 14

[Daniel Veditz [:dveditz]]

fix checked into 1.8 branch

Comment 15

[[e-mail]]

Comment on attachment 197923 [details] [diff] [review]
including merged xpfe/global changes

I checked Editor and it has its own title setting code to, so it's unaffected
by this change.

Comment 16

[[e-mail]]

(In reply to comment #12)
>could we please file a bug to have one and only one place where this code lives
For anyone interested in fixing this follow-up bug, preliminary investigation
suggests that one of the callers of updateTitlebar is unnecessary and the other
caller can be worked around by setting this.contentDocument.title to itself.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Did we ever get this followup filed?

Also, note that this patch caused bug 317750 (though imo that site is just broken).

Comment 18

[Jesse Ruderman]

How did this break bug 317750?  Does the site's JavaScript somehow see the modified title?