60516 - Target name for secondary windows are lost

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GTK Widget, defect, P3)

Description

[rusty.lynch]

With a Gtk embedded browser (like TestGtkEmbed) the target names for secondary
windows seem to be dropped.  For example, if a web page does a 
window.open("some.html","mytarget"), and then later on does a
window.open("someother.html","mytarget"), a new window is created for the
"someother.html" content instead of loading the new content in the existing
window using the name "mytarget".

Comment 1

[rusty.lynch]

Attachment: Simple HTML testcase

Comment 2

[rusty.lynch]

Ok, I think I have found something here. This section of the code is new to me
so I might be making some wrong observations, but for what it's worth here is
what i have noticed.

First of all, when ever we open a new window we open two docshells.  One is for
chrome and the other is for the content.  The chrome docshell is created and
named from nsWebBrowser::Create() which is triggered from realizing the gtk
embed widget.  Also note that in nsWebBrowser::Create() the name for the new
chrome docshell is "".

The problem is that when anyone searches for a docshellitem with a name, we
iterate through the chrome docshells and not the content docshells so that we
are guaranteed not to find anything (unless we are unlucky enough to ask for a
window named "".)

In addition to the behavior discribed in the beginning comment of this report,
we also have another side affect when attempting to open a "_blank", "_new", or
"_content" window from anything originating from HandleLinkClickEvent (like an
anchor with target="_blank".)  The reason is that along the way one of the
methods strip out the fake targets and replace the target name with "".  This
results in loading the content into the chrome docshell which isn't a cool thing
to do.

I think we are doing something wrong in our nsWebBrowser::FindItemWithName() or
what that guy calls, GtkMozEmbedChrome::FindItemWithName(). Although, I my brain
is now completely fried and can't find where the ball is being dropped.

Comment 3

[rusty.lynch]

assigning to blizzard

Comment 4

[rusty.lynch]

Attachment: patch for GtkMozEmbedChrome.cpp

Comment 5

[rusty.lynch]

I've attached a patch for GtkMozEmbedChrome.cpp that seems to do the trick
(except for the javascript: anchors from bug #60522 still block when attempting
to create a new window.)

The idea is that first of all, our first entry point in ::FindItemWithName will
catch the special "_blank" and "_content" cases, and then when we do start
searching through the webbrowser objects, go through the treeOwner to get a
primary content treeitem to start searching from.

I still need to do some testing to make sure this doesn't break any other cases.
(Either way this was an excelent learning experience for me.)

So, Chris, am I close? 

Comment 6

[Christopher Blizzard (:blizzard)]

As far as I know the javascript: bug isn't a problem in the embedding widget.
I've been able to reproduce it in mozilla as well.

I'm looking at this code and will do some testing when my build finishes.
Thanks for doing this, Rusty!

Comment 7

[Christopher Blizzard (:blizzard)]

Attachment: patch #2

Comment 8

[Christopher Blizzard (:blizzard)]

I've uploaded a new patch.  The original patch had the problem where if you had
another unnamed window up it would cause infinite recursion into FindItemWithName().

Comment 9

[rusty.lynch]

Good catch. I've been banging around with the embedded browser most of today and
haven't come across any side effects from the change.

BTW, for anyone querying this report from my guesses about the javascript: url
bug, see bug #55032 for the real story. 

Comment 10

[Christopher Blizzard (:blizzard)]

Adam, can you do a quick review of this code?

Comment 11

[Christopher Blizzard (:blizzard)]

OK, I'm trying to get a review from hyatt/valeski/danm since I think they all
know something about window targets.  Anyone want to look?  It's a pretty small
patch.

Comment 12

[Adam Lock]

Patch looks fine to me, r=adamlock

Comment 13

[Christopher Blizzard (:blizzard)]

Checked in.