Closed
Bug 103638
Opened 23 years ago
Closed 20 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)
Core
Layout: Images, Video, and HTML Frames
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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
danm.moz
:
review+
dveditz
:
superreview+
asa
:
approval-aviary1.0.1+
asa
:
approval1.7.6+
asa
:
approval1.8a6+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
asa
:
approval-aviary1.0.1+
asa
:
approval1.7.6+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
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
Comment 2•23 years ago
|
||
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: 23 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 3•23 years ago
|
||
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.
Reporter | ||
Comment 5•23 years ago
|
||
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
Reporter | ||
Comment 7•23 years ago
|
||
I am wondering why this is still UNCONFIRMED. The testcase still works in 0.9.8. pi
Comment 8•23 years ago
|
||
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•23 years ago
|
||
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
Reporter | ||
Comment 10•23 years ago
|
||
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•23 years ago
|
||
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?
Reporter | ||
Comment 12•23 years ago
|
||
Using Win98SE and 2002032708 I still can produce the problem in the testcase. pi
Reporter | ||
Comment 13•22 years ago
|
||
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•22 years ago
|
||
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.
Updated•22 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Updated•22 years ago
|
Comment 16•22 years ago
|
||
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•22 years ago
|
||
CC'ing danm, who is all over this code.
Updated•22 years ago
|
Whiteboard: [FIX]
Updated•22 years ago
|
Priority: -- → P3
Target Milestone: --- → Future
Updated•22 years ago
|
Whiteboard: [FIX] → [FIX][jk-target]
Comment 18•21 years ago
|
||
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•21 years ago
|
||
Sorry, in the last step I meant: - click any link in the last page...
Comment 20•21 years ago
|
||
*** Bug 215659 has been marked as a duplicate of this bug. ***
Comment 21•21 years ago
|
||
*** Bug 175933 has been marked as a duplicate of this bug. ***
Comment 22•21 years ago
|
||
*** Bug 156461 has been marked as a duplicate of this bug. ***
Comment 23•20 years ago
|
||
*** Bug 244440 has been marked as a duplicate of this bug. ***
Reporter | ||
Updated•20 years ago
|
Updated•20 years ago
|
Whiteboard: [FIX][jk-target] → [FIX][jk-target][sg:fix]
Comment 24•20 years ago
|
||
*** Bug 248008 has been marked as a duplicate of this bug. ***
Comment 25•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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 28•20 years ago
|
||
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.
Assignee | ||
Comment 29•20 years ago
|
||
Assignee | ||
Comment 30•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #170288 -
Flags: superreview?(dveditz)
Attachment #170288 -
Flags: review?(bzbarsky)
Comment 31•20 years ago
|
||
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-
Assignee | ||
Comment 32•20 years ago
|
||
(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.
Assignee | ||
Comment 33•20 years ago
|
||
Attachment #170288 -
Attachment is obsolete: true
Attachment #170310 -
Flags: superreview?(dveditz)
Attachment #170310 -
Flags: review?(danm.moz)
Assignee | ||
Updated•20 years ago
|
Attachment #170288 -
Flags: superreview?(dveditz)
Assignee | ||
Comment 34•20 years ago
|
||
This (or bug 273699 rather, which will be fixed by this bug) should be blocking 1.8a6.
Flags: blocking1.8a6?
Comment 35•20 years ago
|
||
> + // 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.
Updated•20 years ago
|
Flags: blocking1.8a6? → blocking1.8a6+
Updated•20 years ago
|
Target Milestone: Future → mozilla1.8alpha6
Attachment #170310 -
Flags: review?(danm.moz) → review+
Comment 36•20 years ago
|
||
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 37•20 years ago
|
||
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 | ||
Comment 38•20 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 23 years ago → 20 years ago
Resolution: --- → FIXED
Comment 39•20 years ago
|
||
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?
Comment 40•20 years ago
|
||
I really don't think we should be taking that patch on anything claiming to be an API-stable branch...
Comment 41•20 years ago
|
||
This checkin caused bug 278143
And possibly bug 278727?
Comment 44•20 years ago
|
||
*** Bug 280581 has been marked as a duplicate of this bug. ***
Comment 45•20 years ago
|
||
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+
Comment 47•20 years ago
|
||
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•20 years ago
|
||
ccing darin, since our embedding story is involved...
Comment 49•20 years ago
|
||
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•20 years ago
|
||
(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•20 years ago
|
||
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 52•20 years ago
|
||
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•20 years ago
|
||
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...)
Assignee | ||
Comment 54•20 years ago
|
||
I'm porting these changes to the branch...
Assignee | ||
Comment 55•20 years ago
|
||
Updated•20 years ago
|
Whiteboard: [FIX][jk-target][sg:fix] → [FIX][jk-target][sg:fix] need check-in
Assignee | ||
Comment 56•20 years ago
|
||
Assignee | ||
Comment 57•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #174074 -
Attachment is obsolete: true
Attachment #174090 -
Flags: review?(bzbarsky)
Comment 58•20 years ago
|
||
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+
Assignee | ||
Comment 59•20 years ago
|
||
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?
Comment 60•20 years ago
|
||
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!
Assignee | ||
Comment 61•20 years ago
|
||
Cool. I'll merge that in before landing, looks straight forward.
Assignee | ||
Comment 62•20 years ago
|
||
Comment 63•20 years ago
|
||
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+
Updated•20 years ago
|
Keywords: fixed-aviary1.0.1
Assignee | ||
Comment 64•20 years ago
|
||
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 66•20 years ago
|
||
Yeah, that looks ok.
Comment 67•20 years ago
|
||
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
Comment 68•19 years ago
|
||
Attachment #187609 -
Flags: review?(Duecallion)
Attachment #187609 -
Flags: review?(Duecallion) → review?(jst)
Assignee | ||
Comment 69•19 years ago
|
||
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+
Updated•19 years ago
|
Flags: testcase+
Updated•18 years ago
|
Whiteboard: [FIX][jk-target][sg:fix] need check-in → [FIX][jk-target][sg:fix]
Updated•18 years ago
|
Flags: in-testsuite+ → in-testsuite?
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
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.
Description
•