Closed Bug 103638 Opened 19 years ago Closed 15 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.8alpha6

People

(Reporter: 3.14, Assigned: jst)

References

(Blocks 1 open bug, )

Details

(4 keywords, Whiteboard: [FIX][jk-target][sg:fix])

Attachments

(9 files, 3 obsolete files)

60.71 KB, patch
Details | Diff | Splinter Review
78.68 KB, patch
Details | Diff | Splinter Review
84.15 KB, patch
danm.moz
: review+
dveditz
: superreview+
Details | Diff | Splinter Review
112.54 KB, patch
Details | Diff | Splinter Review
109.97 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
115.28 KB, patch
Details | Diff | Splinter Review
120.56 KB, patch
Details | Diff | Splinter Review
115.71 KB, patch
Details | Diff | Splinter Review
120.58 KB, patch
jst
: review+
Details | Diff | Splinter Review
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
-> evaughan
Assignee: pollmann → evaughan
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.
Status: UNCONFIRMED → RESOLVED
Closed: 19 years ago
Resolution: --- → WORKSFORME
I can't see any difference in 0.9.6 (Linux).

pi
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
Unable to reproduce bug in 0.96 (linux)  Opened multiple windows with identical
framsets and identical frames.  All frames opened in the correct window.
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
This bug also happens in the Windows version.

pi
OS: Linux → All
I am wondering why this is still UNCONFIRMED. The testcase still works in 0.9.8.

pi
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.


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)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
Summary: targets with same name in different windows → targets with same name in different windows open in wrong window with javascript
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
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?
Using Win98SE and 2002032708 I still can produce the problem in the testcase.

pi
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
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.
This is horrible.  This is a security issue.
Assignee: eric → jkeiser
Status: NEW → ASSIGNED
Attached patch In-transit patch (DO NOT LAND) (obsolete) — Splinter Review
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).
CC'ing danm, who is all over this code.
Whiteboard: [FIX]
Priority: -- → P3
Target Milestone: --- → Future
Whiteboard: [FIX] → [FIX][jk-target]
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.
Sorry, in the last step I meant:
- click any link in the last page...
*** Bug 215659 has been marked as a duplicate of this bug. ***
Blocks: 218254
*** Bug 175933 has been marked as a duplicate of this bug. ***
*** Bug 156461 has been marked as a duplicate of this bug. ***
*** Bug 244440 has been marked as a duplicate of this bug. ***
Whiteboard: [FIX][jk-target] → [FIX][jk-target][sg:fix]
*** Bug 248008 has been marked as a duplicate of this bug. ***
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...
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?
QA Contact: amar → core.layout.html-frames
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.
Attachment #94729 - Attachment is obsolete: true
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.
Attachment #170288 - Flags: superreview?(dveditz)
Attachment #170288 - Flags: review?(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...
Attachment #170288 - Flags: review?(bzbarsky) → review-
(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.
Attached patch Updated diff -wSplinter Review
Attachment #170288 - Attachment is obsolete: true
Attachment #170310 - Flags: superreview?(dveditz)
Attachment #170310 - Flags: review?(danm.moz)
Attachment #170288 - Flags: superreview?(dveditz)
This (or bug 273699 rather, which will be fixed by this bug) should be blocking
1.8a6.
Flags: blocking1.8a6?
> +            // 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.
Flags: blocking1.8a6? → blocking1.8a6+
Target Milestone: Future → mozilla1.8alpha6
Attachment #170310 - Flags: review?(danm.moz) → review+
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
Attachment #170310 - Flags: superreview?(dveditz) → superreview+
Comment on attachment 170310 [details] [diff] [review]
Updated diff -w

a=asa (on behalf of drivers) for checkin to 1.8a6.
Attachment #170310 - Flags: approval1.8a6+
Assignee: john → jst
Status: ASSIGNED → NEW
Fix checked in.
Status: NEW → RESOLVED
Closed: 19 years ago15 years ago
Resolution: --- → FIXED
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.
Attachment #170310 - Flags: approval1.7.6?
Attachment #170310 - Flags: approval-aviary1.0.1?
I really don't think we should be taking that patch on anything claiming to be
an API-stable branch...
This checkin caused bug 278143
Bug 278916 is another regression from this fix.
Hardware: PC → All
*** Bug 280581 has been marked as a duplicate of this bug. ***
Bug 279495 is another regression.
we're going to have to bite the bullet and get this on the branches, along with
fixes for the regressions.
Flags: blocking1.7.6+
Flags: blocking-aviary1.0.1+
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.
Depends on: 278143, 278916, 279495
ccing darin, since our embedding story is involved...
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)...
(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 on attachment 170310 [details] [diff] [review]
Updated diff -w

a=asa for branches checkins.
Attachment #170310 - Flags: approval1.7.6?
Attachment #170310 - Flags: approval1.7.6+
Attachment #170310 - Flags: approval-aviary1.0.1?
Attachment #170310 - Flags: approval-aviary1.0.1+
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.
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...)
I'm porting these changes to the branch...
Whiteboard: [FIX][jk-target][sg:fix] → [FIX][jk-target][sg:fix] need check-in
Attachment #174074 - Attachment is obsolete: true
Attachment #174090 - Flags: review?(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.
Attachment #174090 - Flags: review?(bzbarsky) → review+
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.
Attachment #174360 - Flags: approval1.7.6?
Attachment #174360 - Flags: approval-aviary1.0.1?
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!
Cool. I'll merge that in before landing, looks straight forward.
Comment on attachment 174360 [details] [diff] [review]
Aviary branch version (diff -w) with bz's issues fixed.

a=asa for branches checkins
Attachment #174360 - Flags: approval1.7.6?
Attachment #174360 - Flags: approval1.7.6+
Attachment #174360 - Flags: approval-aviary1.0.1?
Attachment #174360 - Flags: approval-aviary1.0.1+
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.
Fixed on 1.0.1 and 1.7 branches.
Keywords: fixed1.7.6
Yeah, that looks ok.
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
Status: RESOLVED → VERIFIED
Attachment #187609 - Flags: review?(Duecallion) → review?(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
Attachment #187609 - Flags: review?(jst) → review+
commited into 1.4 branch
Keywords: fixed1.4.5
Blocks: 246498
Flags: testcase+
Whiteboard: [FIX][jk-target][sg:fix] need check-in → [FIX][jk-target][sg:fix]
Flags: in-testsuite+ → in-testsuite?
Product: Core → Core Graveyard
Component: Layout: HTML Frames → Layout: Images
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.