From Georgi: It is possible to spoof the window location with XUL with the browser widget. Check attached file xulspoof.xul which spoofs www.mozilla.org The browser XUL widget with content-primary may introduce other bugs in the future, is it really needed from userland XUL?

FYI whoever tests this: does not appear to work if you open the page in a new tab, but I'm spoofed if I open it in a new window.

Yeah, we should not respect content-primary if it's inside a content webshell. This is quite serious and easy to exploit --- marking blocker for 1.2.

caillon, jag, hyatt, can one of you take this on?

So somewhere around here we need to add an isChrome check: http://lxr.mozilla.org/seamonkey/source/layout/html/document/src/nsFrameFrame.cpp#686 Patch coming up (I hope).

Hmm, so that didn't work. I figured this would work: @@ -683,7 +683,10 @@ nsCOMPtr<nsIDocShellTreeOwner> parentTreeOwner; parentAsItem->GetTreeOwner(getter_AddRefs(parentTreeOwner)); if (parentTreeOwner) { + PRInt32 shellType; + docShellAsItem->GetItemType(&shellType); PRBool is_primary_content = + shellType == nsIDocShellTreeItem::typeChrome && value.EqualsIgnoreCase("content-primary"); parentTreeOwner->ContentShellAdded(docShellAsItem, is_primary_content, I'll have to get some more info on what's going on here I guess.

There we go. I think I got it now.

So it's the patch to nsFrameLoader.cpp that fixed this. I think we should also have the check in nsFrameFrame.cpp, though I'm not quite sure (read: tired and don't wanna work it out right now) how we could trigger the same problem over there.

Great! Let's drive this in.

sr=hyatt

Comment on attachment 103726 [details] [diff] [review] Only accept content-primary for chrome r=caillon provided you tested this. I had a brief look at this a couple weeks ago and tried similar patches, but got a bunch of JS errors and warnings about window.content when loading pages.

Comment on attachment 103726 [details] [diff] [review] Only accept content-primary for chrome - In nsFrameLoader.cpp: - if (isContent) { - // The web shell's type is content. - - docShellAsItem->SetItemType(nsIDocShellTreeItem::typeContent); - } else { - // Inherit our type from our parent webshell. If it is - // chrome, we'll be chrome. If it is content, we'll be - // content. - - docShellAsItem->SetItemType(parentType); - } + PRInt32 shellType = isContent ? nsIDocShellTreeItem::typeContent : parentType; + docShellAsItem->SetItemType(shellType); Leave the comment, in one form or another. sr=jst

I thought the code spoke for itself, but ... // if it isn't content, set it to the parent's type

discussed with dan and jag. plussing for adt and adding nsbeta stuff, linking to meta.

Comment on attachment 103726 [details] [diff] [review] Only accept content-primary for chrome a=dbaron for trunk and 1.0 branch

This caused a regression - bug #176505 - and was backed out.

This fixes the regression for me: Index: nsFrameLoader.cpp =================================================================== RCS file: /cvsroot/mozilla/content/base/src/nsFrameLoader.cpp,v retrieving revision 1.13 diff -u -6 -d -r1.13 nsFrameLoader.cpp --- nsFrameLoader.cpp 24 Oct 2002 04:37:33 -0000 1.13 +++ nsFrameLoader.cpp 24 Oct 2002 20:28:53 -0000 @@ -458,13 +458,13 @@ if (isContent) { nsCOMPtr<nsIDocShellTreeOwner> parentTreeOwner; parentAsItem->GetTreeOwner(getter_AddRefs(parentTreeOwner)); if(parentTreeOwner) { - PRBool is_primary = shellType == nsIDocShellTreeItem::typeChrome && + PRBool is_primary = parentType == nsIDocShellTreeItem::typeChrome && value.Equals(NS_LITERAL_STRING("content-primary")); parentTreeOwner->ContentShellAdded(docShellAsItem, is_primary, value.get()); } }

Duh. You wanna make sure the parent is chrome, not the shell itself. Coming up with a new patch.

Thanks, dbaron.

Comment on attachment 104037 [details] [diff] [review] Fix r=dbaron, except it might be clearer for people modifying both functions in the future if you called the new variable in nsFrameFrame.cpp |parentType| rather than |shellType|.

Comment on attachment 104037 [details] [diff] [review] Fix sr=jst

dbaron: changed. Does a= still stand?

Comment on attachment 104037 [details] [diff] [review] Fix a=dbaron for trunk checkin. (Ask again for branch in a day or two, perhaps?)

That's the plan :-)

ready for the branch?

Yes, very ready :-) Mail was sent to drivers a couple of days ago, hopefully they'll reach it soon.

a=blizzard on behalf of drivers for 1.0.2. Please mark with fixed1.0.2 when you have checked it in and verified1.0.2 once it's been verified.

Did this get checked in on the trunk or not? I can't tell from the comments in the bug.

It got checked in on the trunk. I just checked it in on the branch. Thanks.

Verified on 2002-11-06-branch on Win 2000

Verified on 2002-11-06-branch on Win 2000. Attached test case throws an exception in JS console. This is the correct behavior.

Reopening - the fix for the 1.0 branch appears to be ineffective. We probably need a new patch for the 1.0 branch, and jag is working on that.

Comment on attachment 111108 [details] [diff] [review] Branch fix r=mstoltz

Comment on attachment 111108 [details] [diff] [review] Branch fix sr=bryner

Comment on attachment 111108 [details] [diff] [review] Branch fix a=dbaron for 1.0.x branch checkin

Checked in.

Marking this Verified Fixed Testing against latest branch 2002-02-10-09 on Win2000 After running XULspoof testcase in comment #1, the URL location field will not display http://www.mozilla.org