Last Comment Bug 103638 - targets with same name in different windows open in wrong window with javascript
: targets with same name in different windows open in wrong window with javascript
Status: VERIFIED FIXED
[FIX][jk-target][sg:fix]
: fixed-aviary1.0.1, fixed1.4.5, fixed1.7.6, testcase
Product: Core
Classification: Components
Component: Layout: HTML Frames (show other bugs)
: Trunk
: All All
: P3 normal with 3 votes (vote)
: mozilla1.8alpha6
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
http://piology.org/mozilla/bugs/bugs-...
: 156461 175933 215659 244440 248008 280581 (view as bug list)
Depends on: 278143 278916 279495
Blocks: 218254 246498 269174 270414 271188 272439 sa13129 273984
  Show dependency treegraph
 
Reported: 2001-10-08 06:15 PDT by Boris 'pi' Piwinger
Modified: 2014-04-26 03:21 PDT (History)
29 users (show)
roc: blocking1.7.6+
roc: blocking‑aviary1.0.1+
asa: blocking1.8a6+
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
In-transit patch (DO NOT LAND) (18.08 KB, patch)
2002-08-09 18:53 PDT, John Keiser (jkeiser)
no flags Details | Diff | Splinter Review
Another in-transit patch (also DO NOT LAND) (60.71 KB, patch)
2004-12-10 09:13 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
More along those lines. Fixes the problem, and others, but still not quite there. (78.68 KB, patch)
2004-12-15 17:40 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Proposed fix, bz's fix with window.open() etc fixed. (86.29 KB, patch)
2005-01-04 14:23 PST, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review-
Details | Diff | Splinter Review
Updated diff -w (84.15 KB, patch)
2005-01-04 18:29 PST, Johnny Stenback (:jst, jst@mozilla.com)
danm.moz: review+
dveditz: superreview+
asa: approval‑aviary1.0.1+
asa: approval1.7.6+
asa: approval1.8a6+
Details | Diff | Splinter Review
Branch version of the above, still w/o the suggested new interfaces (85.66 KB, patch)
2005-02-11 11:34 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Branch version w/ new internal "temporary" interfaces (112.54 KB, patch)
2005-02-11 14:01 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Branch version (diff -w) w/ new internal "temporary" interfaces (109.97 KB, patch)
2005-02-11 14:04 PST, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review+
Details | Diff | Splinter Review
Aviary branch version (diff -w) with bz's issues fixed. (115.28 KB, patch)
2005-02-14 22:51 PST, Johnny Stenback (:jst, jst@mozilla.com)
asa: approval‑aviary1.0.1+
asa: approval1.7.6+
Details | Diff | Splinter Review
Complete diff, including the fix for bug 270414 (120.56 KB, patch)
2005-02-15 08:36 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Complete 1.7 branch diff. (115.71 KB, patch)
2005-02-15 16:18 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
patch for 1.4 branch, based on 1.7 branch diff, also merged bug 210560, 212460, 233433 (120.58 KB, patch)
2005-06-28 23:21 PDT, Ginn Chen
jst: review+
Details | Diff | Splinter Review

Description Boris 'pi' Piwinger 2001-10-08 06:15:05 PDT
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.4) Gecko/20010913

When you have (at least;-) two browser windows open, both displaying web pages
using frames, and both framesets containing a frame by the same name, then a
link directed to that frame might open in the wrong window.

My guess is that it always opens in the frame in the first window.

pi
Comment 1 Christopher Hoess (gone) 2001-10-08 08:20:31 PDT
-> evaughan
Comment 2 Keyser Sose 2001-11-09 19:02:21 PST
Marking these all WORKSFORME sorry about lack of response but were very
overloaded here. Only reopen the bug if you can reproduce with the following steps:

1) Download the latest nightly (or 0.9.6 which should be out RSN)
2) Create a new profile
3) test the bug again

If it still occurs go ahead and reopen the bug. Again sorry about no response
were quite overloaded here and understaffed.
Comment 3 Boris 'pi' Piwinger 2001-11-21 05:02:13 PST
I can't see any difference in 0.9.6 (Linux).

pi
Comment 4 Travis 2001-11-24 23:13:40 PST
Unable to reproduce bug in 0.96 (linux)  Opened multiple windows with identical
framsets and identical frames.  All frames opened in the correct window.
Comment 5 Boris 'pi' Piwinger 2001-11-25 07:34:50 PST
Travis, you are right. I investigated further. This bug only happens with
JavaScript. I created a testcase:
http://www.logic.univie.ac.at/~3.14/mozilla-bugs-frames/

pi
Comment 6 Boris 'pi' Piwinger 2001-11-25 08:10:57 PST
This bug also happens in the Windows version.

pi
Comment 7 Boris 'pi' Piwinger 2002-02-06 05:54:54 PST
I am wondering why this is still UNCONFIRMED. The testcase still works in 0.9.8.

pi
Comment 8 Michael Lefevre 2002-02-15 20:11:39 PST
i can confirm this - win2000 2002021503.

note that you need to have script enabled and "allow scripts to... open
unrequested windows" checked for it to work.


Comment 9 Michael Lefevre 2002-03-01 05:53:28 PST
still seems to happen checking the testcase with last night's win2k build. 
confirming, and updating summary and keywords.

(NB this is the first bug I've confirmed since I got privileges - please apply
cluebat to me if I've messed this up)
Comment 10 Boris 'pi' Piwinger 2002-03-12 07:05:01 PST
Still there in 0.9.9. Annoying for frames with typical names like "top", "left",
"right", "main". Having two windows open with a frameset in each can be really
confusing.

pi
Comment 11 Michael Lefevre 2002-03-27 18:01:11 PST
having observed a similar bug with wrong frame targets being fixed, i just
rechecked this one.  testcase now WFM with win2k build 2002032708.

Pi - could you try again with the latest nightly build, and see if you can
confirm that this has been fixed?
Comment 12 Boris 'pi' Piwinger 2002-03-28 01:37:14 PST
Using Win98SE and 2002032708 I still can produce the problem in the testcase.

pi
Comment 13 Boris 'pi' Piwinger 2002-06-01 14:23:45 PDT
Still happening (no crash, just double open) in Mozilla/5.0 (Windows; U; Win98;
en-US; rv:1.0.0+) Gecko/20020530.

I am just wondering that target="_new" in the testcase does not work, so you
have to make sure you open in new window by other means.

pi
Comment 14 Reimar Döffinger 2002-06-24 06:25:18 PDT
target="_new" doesn't work because it should be target="_blank".

I also just verified the bug with Mozilla/5.0 (Windows; U; Windows NT 5.0;
en-US; rv:1.1a+) Gecko/20020623, it's still there.

Btw.: Some of the problem seems to lie in the specifications, as it allows to
replace frames from a completely different source, so that some webpages write
into another one's frame although they actually wanted to open a new named
window with javascript. This special problem also exists with IE.
Comment 15 John Keiser (jkeiser) 2002-07-31 16:50:15 PDT
This is horrible.  This is a security issue.
Comment 16 John Keiser (jkeiser) 2002-08-09 18:53:11 PDT
Created attachment 94729 [details] [diff] [review]
In-transit patch (DO NOT LAND)

Just posting this as a snapshot.  I reworked FindTarget a little to be simpler,
but the right way to solve this bug (I think) is make
nsWindowWater::OpenWindowJS() call DocShell's FindTarget (or a deeper method in
DocShell).
Comment 17 John Keiser (jkeiser) 2002-08-09 18:54:25 PDT
CC'ing danm, who is all over this code.
Comment 18 Bernat 2003-06-12 08:33:50 PDT
This has just happened to me with Mozilla1.4rc1 on linux, I haven't tested
latest nightly builds.

Check this:
- start mozilla
- open http://www.cyberpathway.com/
- new navigator window, open http://www.netfort.gr.jp/~dancer/frameindex.html.en
- click any target in the last page (they have target="main") and it'll open in
the other window frame.

It can be dangerous.
Comment 19 Bernat 2003-06-12 08:38:33 PDT
Sorry, in the last step I meant:
- click any link in the last page...
Comment 20 Alfonso Martinez 2003-08-09 15:01:52 PDT
*** Bug 215659 has been marked as a duplicate of this bug. ***
Comment 21 Martijn Ras 2003-12-18 14:42:18 PST
*** Bug 175933 has been marked as a duplicate of this bug. ***
Comment 22 Asa Dotzler [:asa] 2004-03-09 13:03:18 PST
*** Bug 156461 has been marked as a duplicate of this bug. ***
Comment 23 Boris Zbarsky [:bz] 2004-05-23 10:36:01 PDT
*** Bug 244440 has been marked as a duplicate of this bug. ***
Comment 24 R.K.Aa. 2004-06-21 15:52:56 PDT
*** Bug 248008 has been marked as a duplicate of this bug. ***
Comment 25 Boris Zbarsky [:bz] 2004-07-12 15:54:04 PDT
Boris, any chance of reposting your original testcase?  I'm not sure the issue
raised in comment 18 is the same as the original bug here...
Comment 26 Boris Zbarsky [:bz] 2004-12-04 14:13:59 PST
OK, I was just thinking about this... here is my proposal:

1)  Consolidate the logic for finding things by name in one or two methods on
    nsPIWindowWatcher.
2)  Use those methods in nsDocShell::FindTarget,
    nsDocShellTreeOwner::FindItemWithName, nsChromeTreeOwner::FindItemWithName,
    nsContentTreeOwner::FindItemWithName, and windowwatcher itself.

If this seems like a reasonable approach, I'd be willing to do it... the problem
that worries me is embedding -- right now all this code just uses
nsIWindowWatcher, so people who override the windowwatcher but don't implement
nsPIWindowWatcher can have this working.  Is that a service we expect embeddors
to override?

Failing this, the simple fix for this bug is to make OpenWindowJS start the
search at aParent's docshell, instead of the corresponding docshell treeowner...

Danm?  Thoughts?
Comment 27 Boris Zbarsky [:bz] 2004-12-10 09:13:43 PST
Created attachment 168420 [details] [diff] [review]
Another in-transit patch (also DO NOT LAND)

This does fix the symptoms in this bug, but it's nowhere close to being done
yet... note the XXX comments, and the 4 different places that walk the window
list still need refactoring.
Comment 28 Boris Zbarsky [:bz] 2004-12-10 09:15:46 PST
Comment on attachment 168420 [details] [diff] [review]
Another in-transit patch (also DO NOT LAND)

Note also that this has hooks for fixing bug 273699 but they are not yet used.
Comment 29 Johnny Stenback (:jst, jst@mozilla.com) 2004-12-15 17:40:06 PST
Created attachment 168821 [details] [diff] [review]
More along those lines. Fixes the problem, and others, but still not quite there.
Comment 30 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-04 14:23:07 PST
Created attachment 170288 [details] [diff] [review]
Proposed fix, bz's fix with window.open() etc fixed.
Comment 31 Boris Zbarsky [:bz] 2005-01-04 16:40:09 PST
Comment on attachment 170288 [details] [diff] [review]
Proposed fix, bz's fix with window.open() etc fixed.

First of all, I'd love to see danm's comments on this patch too.  As it is,
this feels too much like reviewing my own code.  ;)

>Index: docshell/base/nsDocShell.cpp

>+// Validate window targets to prevent frameset spoofing
>+static PRBool gValidateOrigin = PR_TRUE;

Is there a reason to make this a global other than reducing the size of
docshell?  This will mean that changing the pref and then opening a new window
will change behavior in all existing windows, for example.  If we do make this
a global, I'd prefer we only wrote it once, not on every docshell creation.

>+ValidateOrigin(nsIDocShellTreeItem* aOriginTreeItem,
>+               nsIDocShellTreeItem* aTargetTreeItem)

So the only substantive change here is checking for UniversalBrowserWrite, yes?

>+    printf("fofofof\n");

Take this out?	;)

> nsDocShell::FindItemWithName(const PRUnichar * aName,
>+        NS_ASSERTION(!aOriginalRequestor, "Bogus original requestor");

I don't think this assert is correct.  There can be calls into this code with
aOriginalRequestor set but aRequestor not set.

>+        if (foundItem && ItemIsActive(foundItem)) {

I'm not sure this ItemIsActive() check is correct.  That is, if someone
requests the "_self" window on a now-closed window, I think it's more correct
to open a new window or just do the load in the closed window than to look for
a random accessible window named "_self"...  Then again, I can see arguments
both ways here.  I'd like to know what danm thinks.

>+    if (mName.Equals(aName) && ItemIsActive(this) &&
>+        !CanAccessItem(this, aOriginalRequestor)) {
>+        NS_ADDREF(*_retval = this);
>         return NS_OK;

The '!' on CanAccessItem() here is wrong.

> nsDocShell::CheckLoadingPermissions()
> {
>+    // This method checks whether ther caller may load content into

s/ther/the

>+    if (!gValidateOrigin || !IsFrame()) {
>         // Origin validation was turned off, or we're not a frame.
>         // Permit all loads.
> 
>         return rv;
>     }

Do we need to worry about the toplevel window case here, actually?

>Index: docshell/base/nsIDocShellTreeItem.idl
>+	const long typeContentDialog=4;     // typeContentDialog must equal 4

This doesn't seem relevant to this patch.

>+	aOriginalRequestor - The original treeitem that made the request, if any.
>+    	This is used to ensure that we don't run into cross-site issues.

The second line probably needs two tabs instead of all the spaces to be like
the rest of the interface.... similar for the other interfaces this patch
touches.  Maybe not worth worrying about pending de-tabification of the
interface files, though.

>Index: dom/src/base/nsGlobalWindow.cpp

> nsGlobalWindow::CheckOpenAllow(PopupControlState aAbuseLevel,
>         // _main is an IE target which should be case-insensitive but isn't
[etc.]

It's not completely clear why this code treates allowSelf and allowExtant
differently, and why allowSelf corresponds to something other than "this very
window".... More on this in a sec.

>+            caller->FindItemWithName(PromiseFlatString(aName).get(),
>+                                     NS_STATIC_CAST(nsIDOMWindow *, this),
>+                                     caller, getter_AddRefs(namedItem));

This is wrong, imo.  We want to be calling FindItemWithName on mDocShell, and
passing |caller| as the original requestor (this does the latter but not the
former).  Further, we should be passing a null aRequestor; otherwise the
"_parent" target, for example, will not get found properly.

I think the way we should do this function is as follows:

1)  If mDocShell is gone, go to window watcher and bail out.
2)  If mDocShell is around, ask it for the named item.
3)  Do checks on how the return is related to mDocShell to determine
    whether we want to do allowSelf vs. allowExtant.

>+              wwatch->GetWindowByName(PromiseFlatString(aName).get(), this,
>+                                      getter_AddRefs(namedWindow));

Again, I think the requestor should be null here, since this is an entry into
the the window-finding algorithm.  No matter what we do, this code should be as
close as possible to the code in OpenInternal.	Maybe we can even factor it out
into a shared method?

>@@ -4621,64 +4671,61 @@ nsGlobalWindow::OpenInternal(const nsASt

>+    docShell->FindItemWithName(PromiseFlatString(aName).get(),
>+                               NS_STATIC_CAST(nsIDOMWindow *, this),
>+                               callerItem, getter_AddRefs(namedWindow));

Don't pass |this| as aRequestor; pass null.

>Index: embedding/components/windowwatcher/src/nsWindowWatcher.cpp
>@@ -516,53 +516,49 @@ nsWindowWatcher::OpenWindowJS(nsIDOMWind
>+    if (!callerItem) {
>+      callerItem = do_QueryInterface(aParent);
>+    }

You want GetWindowTreeItem() here -- QI on a window will never give you an
nsIDocShellTreeItem.

>+      parentItem->FindItemWithName(name.get(), aParent, callerItem,
>+                                   getter_AddRefs(newDocShellItem));

The aRequestor needs to be null here.

>@@ -1049,26 +1045,27 @@ nsWindowWatcher::GetWindowByName(const P
>+      // XXXbz sort out original requestor?
>+      docShellTreeItem->FindItemWithName(aTargetName, nsnull, nsnull,
>                                          getter_AddRefs(treeItem));

In particular, do we want to get it off the JS stack if possible?  Or can we
just assume that people calling windowwatcher methods directly like that should
be allowed to do whatever they want?

>Index: webshell/tests/viewer/nsWebBrowserChrome.cpp

The method being modified in this file seems to be unused and in need of
removing...
Comment 32 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-04 18:13:32 PST
(In reply to comment #31)
> (From update of attachment 170288 [details] [diff] [review] [edit])
> First of all, I'd love to see danm's comments on this patch too.  As it is,
> this feels too much like reviewing my own code.  ;)

Yeah, good point :) I'll request danm's review on the next version of this patch
(but feel free to keep commenting, of course).

> >+// Validate window targets to prevent frameset spoofing
> >+static PRBool gValidateOrigin = PR_TRUE;
> 
> Is there a reason to make this a global other than reducing the size of
> docshell?  This will mean that changing the pref and then opening a new window
> will change behavior in all existing windows, for example.  If we do make this
> a global, I'd prefer we only wrote it once, not on every docshell creation.

I changed this so that we only read this pref on startup, I really don't care to
have this pref be live, heck, we don't even set this pref in our code...

> >+ValidateOrigin(nsIDocShellTreeItem* aOriginTreeItem,
> >+               nsIDocShellTreeItem* aTargetTreeItem)
> 
> So the only substantive change here is checking for UniversalBrowserWrite, yes?

Yeah, I'll attach a diff -w next time.

> > nsDocShell::FindItemWithName(const PRUnichar * aName,
> >+        NS_ASSERTION(!aOriginalRequestor, "Bogus original requestor");
> 
> I don't think this assert is correct.  There can be calls into this code with
> aOriginalRequestor set but aRequestor not set.

I removed the assert alltogether.

> >+        if (foundItem && ItemIsActive(foundItem)) {
> 
> I'm not sure this ItemIsActive() check is correct.  That is, if someone
> requests the "_self" window on a now-closed window, I think it's more correct
> to open a new window or just do the load in the closed window than to look for
> a random accessible window named "_self"...  Then again, I can see arguments
> both ways here.  I'd like to know what danm thinks.

Yeah, good point. I changed this to not check if the item is active, so that
means we'll end up loading content into a closed window in odd edge cases, but
that's what the page requested, so...

> >+    if (mName.Equals(aName) && ItemIsActive(this) &&
> >+        !CanAccessItem(this, aOriginalRequestor)) {
> >+        NS_ADDREF(*_retval = this);
> >         return NS_OK;
> 
> The '!' on CanAccessItem() here is wrong.

Duh.

> >+    if (!gValidateOrigin || !IsFrame()) {
> >         // Origin validation was turned off, or we're not a frame.
> >         // Permit all loads.
> > 
> >         return rv;
> >     }
> 
> Do we need to worry about the toplevel window case here, actually?

I don't *think* we need to. Since we won't get here in the targetted link or
"targetted" window.open() cases, the only case we can get here is if the caller
opened the window (or if a reference to the window was leaked to another window
somehow), and in that case we'd permit the load anyways. So I don't think we
need to worry about that case here.

> >+            caller->FindItemWithName(PromiseFlatString(aName).get(),
> >+                                     NS_STATIC_CAST(nsIDOMWindow *, this),
> >+                                     caller, getter_AddRefs(namedItem));
> 
> This is wrong, imo.  We want to be calling FindItemWithName on mDocShell, and
> passing |caller| as the original requestor (this does the latter but not the
> former).  Further, we should be passing a null aRequestor; otherwise the
> "_parent" target, for example, will not get found properly.

Yeah, agreed.

> I think the way we should do this function is as follows:
> 
> 1)  If mDocShell is gone, go to window watcher and bail out.
> 2)  If mDocShell is around, ask it for the named item.
> 3)  Do checks on how the return is related to mDocShell to determine
>     whether we want to do allowSelf vs. allowExtant.

Yes, done.

> >+              wwatch->GetWindowByName(PromiseFlatString(aName).get(), this,
> >+                                      getter_AddRefs(namedWindow));
> 
> Again, I think the requestor should be null here, since this is an entry into
> the the window-finding algorithm.  No matter what we do, this code should be as
> close as possible to the code in OpenInternal.	Maybe we can even factor it out
> into a shared method?

Done.

> >@@ -1049,26 +1045,27 @@ nsWindowWatcher::GetWindowByName(const P
> >+      // XXXbz sort out original requestor?
> >+      docShellTreeItem->FindItemWithName(aTargetName, nsnull, nsnull,
> >                                          getter_AddRefs(treeItem));
> 
> In particular, do we want to get it off the JS stack if possible?  Or can we
> just assume that people calling windowwatcher methods directly like that should
> be allowed to do whatever they want?

IMO the only likely callers of this method should be allowed to do whatever they
want. I added a comment to the IDL to reflect this.

> 
> >Index: webshell/tests/viewer/nsWebBrowserChrome.cpp
> 
> The method being modified in this file seems to be unused and in need of
> removing...

Yeah, it's even in an #if 0 block... removed.
Comment 33 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-04 18:29:58 PST
Created attachment 170310 [details] [diff] [review]
Updated diff -w
Comment 34 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-04 18:44:56 PST
This (or bug 273699 rather, which will be fixed by this bug) should be blocking
1.8a6.
Comment 35 Boris Zbarsky [:bz] 2005-01-04 21:32:48 PST
> +            // item since all the names we've dealt with so far area

s/area/are/

>> 3)  Do checks on how the return is related to mDocShell to determine
>>     whether we want to do allowSelf vs. allowExtant.
>
>Yes, done

Looks like you're still special-casing window names here, though (_top, _self,
etc).  I was sorta trying to eliminate that.  :)  I guess we can leave that for
now, and I'll fix it when I finish cleaning this stuff up...

Other than that, looks good to me at this point.
Comment 36 Daniel Veditz [:dveditz] 2005-01-10 00:29:02 PST
Comment on attachment 170310 [details] [diff] [review]
Updated diff -w

>+static PRBool
>+ValidateOrigin(nsIDocShellTreeItem* aOriginTreeItem,
 ...
>+    nsresult rv =
>+        securityManager->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal));
>+    NS_ENSURE_SUCCESS(rv, rv);

I assume this should return PR_FALSE? As-is you're sending back true.

other than that, sr=dveditz
Comment 37 Asa Dotzler [:asa] 2005-01-10 14:09:32 PST
Comment on attachment 170310 [details] [diff] [review]
Updated diff -w

a=asa (on behalf of drivers) for checkin to 1.8a6.
Comment 38 Johnny Stenback (:jst, jst@mozilla.com) 2005-01-11 11:40:56 PST
Fix checked in.
Comment 39 Mike Kowalski 2005-01-11 15:21:31 PST
Comment on attachment 170310 [details] [diff] [review]
Updated diff -w

this should be fixed for 1.7.6 and if there is a ff 1.0.1.
Comment 40 Boris Zbarsky [:bz] 2005-01-11 16:22:07 PST
I really don't think we should be taking that patch on anything claiming to be
an API-stable branch...
Comment 41 Dan M 2005-01-21 11:59:00 PST
This checkin caused bug 278143
Comment 42 Stephen Donner [:stephend] 2005-01-21 12:24:51 PST
And possibly bug 278727?
Comment 43 Simon Fraser 2005-01-26 18:09:45 PST
Bug 278916 is another regression from this fix.
Comment 44 Phil Ringnalda (:philor) 2005-01-31 15:03:20 PST
*** Bug 280581 has been marked as a duplicate of this bug. ***
Comment 45 Boris Zbarsky [:bz] 2005-02-02 10:58:24 PST
Bug 279495 is another regression.
Comment 46 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-02-02 12:49:13 PST
we're going to have to bite the bullet and get this on the branches, along with
fixes for the regressions.
Comment 47 Boris Zbarsky [:bz] 2005-02-02 19:45:45 PST
Adding some of the regressions to the "depends on" list, so we can keep track of
them.

Note that some of the bugs in the "blocks" list are also regressions from this
bug, as I recall.  Would be good to sort out which of them are...

Also, it'd be good to have all the regressions fixed on trunk and those fixes
tested before we put this on any branches.

What worries me most is that this patch actually changes some embedding-type
interfaces, and the regression fixes change the behavior of nsWebBrowser in some
ways....  Landing that sort of thing on api-stable branches is really scary, to me.
Comment 48 Boris Zbarsky [:bz] 2005-02-05 00:19:54 PST
ccing darin, since our embedding story is involved...
Comment 49 Boris Zbarsky [:bz] 2005-02-06 16:15:53 PST
Perhaps the right way to do this for branch is to add some branch-only apis that
have the new method exposed on them and then call those from all our internal
code (so basically, on branch we would be calling
nsPIDocShellTreeItem.findChildWithName() or something)...
Comment 50 Eric Dorland 2005-02-09 11:18:12 PST
(In reply to comment #39)
> (From update of attachment 170310 [details] [diff] [review] [edit])
> this should be fixed for 1.7.6 and if there is a ff 1.0.1.
> 

Is there a ff 1.0 version of this patch floating around?
Comment 51 Asa Dotzler [:asa] 2005-02-10 12:53:06 PST
Comment on attachment 170310 [details] [diff] [review]
Updated diff -w

a=asa for branches checkins.
Comment 52 Christopher Aillon (sabbatical, not receiving bugmail) 2005-02-10 13:22:08 PST
Comment on attachment 170310 [details] [diff] [review]
Updated diff -w

I just approved the regression fixes for branch landing.  Please make sure
those also land with this patch.
Comment 53 Boris Zbarsky [:bz] 2005-02-10 13:26:37 PST
Note that the behavior in bug 270414 also regressed somewhat from this patch
(and that the fix there wasn't even nominated for the branches till now...)
Comment 54 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-11 10:10:38 PST
I'm porting these changes to the branch...
Comment 55 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-11 11:34:16 PST
Created attachment 174074 [details] [diff] [review]
Branch version of the above, still w/o the suggested new interfaces
Comment 56 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-11 14:01:35 PST
Created attachment 174089 [details] [diff] [review]
Branch version w/ new internal "temporary" interfaces
Comment 57 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-11 14:04:32 PST
Created attachment 174090 [details] [diff] [review]
Branch version (diff -w) w/ new internal "temporary" interfaces
Comment 58 Boris Zbarsky [:bz] 2005-02-11 22:24:14 PST
Comment on attachment 174090 [details] [diff] [review]
Branch version (diff -w) w/ new internal "temporary" interfaces

>Index: docshell/base/nsDocShell.cpp
>+SameOrSubdomainOfTarget(nsIURI* aOriginURI, nsIURI* aTargetURI,

I guess the patch in bug 270414 hasn't been approved yet, eh?  I'll merge that
if/when drivers get to it....

>+nsDocShell::FindItemWithName(const PRUnichar * aName,
>+                             nsISupports * aRequestor,
>+                             nsIDocShellTreeItem * aOriginalRequestor,
>+                             nsIDocShellTreeItem ** _retval)

I'd really prefer names for the "Tmp" methods that are distinct from the other
ones; otherwise calls from JS (with interface flattening) are a bit of a
problem... Maybe just stick "Tmp" on these method names too?

>+nsDocShell::FindChildWithName(const PRUnichar * aName,

There is a check for *aName that was added in bug 278143 that seems to be
missing here.

>Index: docshell/base/nsIDocShellTreeItem.idl
>+	new signiature and details.

"signature"

>Index: docshell/base/nsIDocShellTreeNode.idl
>+	Deprecate method signature, see nsIDocShellTreeNodeTmp for details

"Deprecated"

>Index: docshell/base/nsIDocShellTreeNodeTmp.idl
>+	nsIDocShellTreeItem findChildWithName(in wstring aName,
>+										  in boolean aRecurse,

Fix indent?

>Index: embedding/browser/webBrowser/nsDocShellTreeOwner.cpp
>+nsDocShellTreeOwner::FindItemWithName(const PRUnichar* aName,
>+    if (mTreeOwner != reqAsTreeOwner) {
>+      nsCOMPtr<nsIDocShellTreeOwnerTmp> owner(do_QueryInterface(mTreeOwner));

mTreeOwner here can be set by the embeddor, no?  At least I suspect it can...
So we need to eithe not change this part, or fall back on nsIDocShellTreeOwner
if that QI fails.

In fact, that may be a good idea inside nsDocShell too, when calling up to the
treeowner, if the QI fails...

>   // finally, failing everything else, search all windows, if we're not already
>   if (mWebBrowser->mDocShellAsItem != aRequestor)
>-    return FindItemWithNameAcrossWindows(aName, aFoundItem);
>+    return FindItemWithNameAcrossWindows(aName, aOriginalRequestor, aFoundItem);

This is missing the changes for bug 278916.  Are we doing those in a separate
patch?	(I'm fine either way as long as the changes make it to branch...)

>Index: embedding/components/windowwatcher/src/nsWindowWatcher.cpp
>+nsWindowWatcher::FindItemWithName(const PRUnichar* aName,
>+          rv = treeItemTmp->FindItemWithName(aName, treeItem,
>+                                             aOriginalRequestor, aFoundItem);

Hmm... This matches trunk, but the code is wrong (and has been all along, even
before the patches for this bug).  File a bug on me about this?

>Index: xpfe/appshell/src/nsChromeTreeOwner.cpp

This is missing the changes for bug 279495 (though you merged in the docshell
part of this patch).  Want to roll those in, or separate patch?

r=bzbarsky with these issues addressed.
Comment 59 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-14 22:51:42 PST
Created attachment 174360 [details] [diff] [review]
Aviary branch version (diff -w) with bz's issues fixed.

This fixes all that, and also rolls in the changes in bug 278143, bug 278143,
and bug 279495. I'll land this tomorrow (pending approval), but bz, if you've
got some cycles, please have one more look.
Comment 60 Boris Zbarsky [:bz] 2005-02-14 23:04:13 PST
Note that that the patch in bug 270414 has approval now; it should really go on
branch too.  If you have the cycles to do it, that would rock; it should be a
very simple merge.  Otherwise I'll pull a branch tree....

Other than that, looks good to me!  Thanks for doing this!
Comment 61 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-14 23:18:28 PST
Cool. I'll merge that in before landing, looks straight forward.
Comment 62 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-15 08:36:01 PST
Created attachment 174382 [details] [diff] [review]
Complete diff, including the fix for bug 270414
Comment 63 Asa Dotzler [:asa] 2005-02-15 08:47:54 PST
Comment on attachment 174360 [details] [diff] [review]
Aviary branch version (diff -w) with bz's issues fixed.

a=asa for branches checkins
Comment 64 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-15 16:18:04 PST
Created attachment 174429 [details] [diff] [review]
Complete 1.7 branch diff.

bz, please look over the nsDocShell and nsGlobalWindow changes here, I had to
do some hand merging there since the whole slew of code that deals with the
prefs about where to open new windows doesn't exist on the 1.7 branch.
Comment 65 Johnny Stenback (:jst, jst@mozilla.com) 2005-02-15 16:36:12 PST
Fixed on 1.0.1 and 1.7 branches.
Comment 66 Boris Zbarsky [:bz] 2005-02-15 16:44:26 PST
Yeah, that looks ok.
Comment 67 Jay Patel [:jay] 2005-02-18 20:30:42 PST
Verified Fixed.  Testcase http://piology.org/mozilla/bugs/bugs-frames/ no longer
opens frames in the wrong window.

Aviary 1.0.1 branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5)
Gecko/20050218 Firefox/1.0

M176 branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6)
Gecko/20050217
Comment 68 Ginn Chen 2005-06-28 23:21:40 PDT
Created attachment 187609 [details] [diff] [review]
patch for 1.4 branch, based on 1.7 branch diff, also merged bug 210560, 212460, 233433
Comment 69 Johnny Stenback (:jst, jst@mozilla.com) 2005-06-30 17:57:09 PDT
Comment on attachment 187609 [details] [diff] [review]
patch for 1.4 branch, based on 1.7 branch diff, also merged bug 210560, 212460, 233433

r=jst
Comment 70 Ginn Chen 2005-06-30 20:32:38 PDT
commited into 1.4 branch

Note You need to log in before you can comment on or make changes to this bug.