103638 - targets with same name in different windows open in wrong window with javascript

Status: VERIFIED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P3)

Description

[Boris 'pi' Piwinger]

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)]

-> evaughan

Comment 2

[Keyser Sose]

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]

I can't see any difference in 0.9.6 (Linux).

pi

Comment 4

[Travis]

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]

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]

This bug also happens in the Windows version.

pi

Comment 7

[Boris 'pi' Piwinger]

I am wondering why this is still UNCONFIRMED. The testcase still works in 0.9.8.

pi

Comment 8

[Michael Lefevre]

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]

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]

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]

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]

Using Win98SE and 2002032708 I still can produce the problem in the testcase.

pi

Comment 13

[Boris 'pi' Piwinger]

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]

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)]

This is horrible.  This is a security issue.

Comment 16

[John Keiser (jkeiser)]

Attachment: 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)]

CC'ing danm, who is all over this code.

Comment 18

[Bernat]

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]

Sorry, in the last step I meant:
- click any link in the last page...

Comment 20

[Alfonso Martinez]

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

Comment 21

[Martijn Ras]

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

Comment 22

[Asa Dotzler [:asa]]

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

Comment 23

[Boris Zbarsky [:bzbarsky]]

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

Comment 24

[R.K.Aa.]

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

Comment 25

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Attachment: 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 [:bzbarsky]]

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)]

Attachment: More along those lines. Fixes the problem, and others, but still not quite there.

Comment 30

[Johnny Stenback (:jst)]

Attachment: Proposed fix, bz's fix with window.open() etc fixed.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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)]

(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)]

Attachment: Updated diff -w

Comment 34

[Johnny Stenback (:jst)]

This (or bug 273699 rather, which will be fixed by this bug) should be blocking
1.8a6.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> +            // 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]]

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]]

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)]

Fix checked in.

Comment 39

[Mike Kowalski]

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 [:bzbarsky]]

I really don't think we should be taking that patch on anything claiming to be
an API-stable branch...

Comment 41

[Dan M]

This checkin caused bug 278143

Comment 42

[Stephen Donner [:stephend] Not actively reading bugmail]

And possibly bug 278727?

Comment 43

[Simon Fraser [no longer active]]

Bug 278916 is another regression from this fix.

Comment 44

[Phil Ringnalda (:philor)]

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

Comment 45

[Boris Zbarsky [:bzbarsky]]

Bug 279495 is another regression.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 [:bzbarsky]]

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 [:bzbarsky]]

ccing darin, since our embedding story is involved...

Comment 49

[Boris Zbarsky [:bzbarsky]]

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]

(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]]

Comment on attachment 170310 [details] [diff] [review]
Updated diff -w

a=asa for branches checkins.

Comment 52

[Christopher Aillon (sabbatical, not receiving bugmail)]

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 [:bzbarsky]]

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)]

I'm porting these changes to the branch...

Comment 55

[Johnny Stenback (:jst)]

Attachment: Branch version of the above, still w/o the suggested new interfaces

Comment 56

[Johnny Stenback (:jst)]

Attachment: Branch version w/ new internal "temporary" interfaces

Comment 57

[Johnny Stenback (:jst)]

Attachment: Branch version (diff -w) w/ new internal "temporary" interfaces

Comment 58

[Boris Zbarsky [:bzbarsky]]

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)]

Attachment: 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 [:bzbarsky]]

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)]

Cool. I'll merge that in before landing, looks straight forward.

Comment 62

[Johnny Stenback (:jst)]

Attachment: Complete diff, including the fix for bug 270414

Comment 63

[Asa Dotzler [:asa]]

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)]

Attachment: 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)]

Fixed on 1.0.1 and 1.7 branches.

Comment 66

[Boris Zbarsky [:bzbarsky]]

Yeah, that looks ok.

Comment 67

[Jay Patel [:jay]]

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]

Attachment: patch for 1.4 branch, based on 1.7 branch diff, also merged bug 210560, 212460, 233433

Comment 69

[Johnny Stenback (:jst)]

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]

commited into 1.4 branch