User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 Build ID: 20130116073211 Steps to reproduce: Two examples of the issue: 1) Try to navigate another sandboxed iframe, using its name, from a sandboxed iframe that is its sibling. 2) Try to navigate top, using its name, from a sandboxed iframe that does not have allow-top-navigation specified. I have attached a patch to the existing navigation tests for the sandboxing to demonstrate both. Actual results: The navigations were allowed in both cases. Expected results: The navigations should have been blocked in both cases because of the sandboxing on the source browsing context. I believe case 1, should have failed trying to navigate any target browsing context that wasn't one nested within the source browsing context.

Hi bz and bholley, Ian Melven has advised me to copy you in to this bug to ask your advice. I have been working with Ian on bug 766282 to add allow-popups functionality to sandboxes. Hopefully, I've explained and demonstrated the problem clearly enough in the description. This behaviour would normally be allowed as per http://www.w3.org/TR/2012/CR-html5-20121217/browsers.html#security-nav However, I believe the sandboxing rules should override this, see http://www.w3.org/TR/2012/CR-html5-20121217/browsers.html#sandboxing and http://www.w3.org/TR/2012/CR-html5-20121217/browsers.html#navigating-across-documents If I am correct, I wonder if it would be useful to ask for a clarification to be added to section 5.1.4, to state that the sandboxing rules override these rules. That aside, and assuming you think it is a problem, I believe the cause is in nsDocShell::FindItemWithName. The sandboxing rules are checked after the special keywords are processed, but before any searches for an actual browsing context name. These were some initial suggestions for a solution, I put to Ian: 1) Move the sandbox checks outside of FindItemWithName, probably with a function to do it, so it can easily be called from several places. 2) Move them to CanAccessItem. 3) Adding a new function in the place of CanAccessItem, which calls CanAccessItem and a new function to do the sandbox checks. 4) Restructure FindItemWithName to not call functions further down that return straight out and move the sandbox checks right to the end of the function. On reflection, as I've been preparing to file this bug, I think options 2 and 3 won't work, because CanAccessItem just returns a bool and doesn't stop the search. I believe the spec suggests that, if the found item is not allowed due to sandboxing the search should be aborted with no browsing context selected (and a new one not created). The thing I don't like about option 1 is that the sandboxing checks would have to be done in multiple places. It would also be easier for this to be missed out in future modifications. Option 4, is also tricky because of the recursive nature of the search, although I think the sandboxing checks could be done at the end of the function, when aRequestor is null (assuming some of the internal calls are prevented from returning straight out). I'm sure you might have other better options as well. Thanks for your help, Bob

Allowing either of these navigations would defeat the intent of sandboxing, I think.

(In reply to Bob Owen from comment #0) > > 2) Try to navigate top, using its name, from a sandboxed iframe that does > not have allow-top-navigation specified. To be explicit, this means navigating top using the frame's actual name, not "_top" - navigating using '_top' should be correctly blocked.

> I wonder if it would be useful to ask for a clarification to be added to section 5.1.4, Yes! There's a nice non-normative table at http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#browsing-context-names but I see nothing in the normative text that would give the behavior in that table. So the spec definitely needs fixing here. In terms of what the right solution is.... Given the desired behavior it seems like the right thing is to either check in FindItemWithName in the null aRequestor case (possibly by creating a DoFindItemWithName that does all the work and then checking the return value in FindItemWithName) or to check in the caller(s) of FindItemWithName. There aren't that many of those, actually. But I'd prefer the "check in FindItemWithName if aRequestor is null" approach.

And one other note. The spec's table misses a case: "name that exists and is a descendant of some ancestor but not an ancestor or descendant of current" (basically uncle/aunt/nephew/niece/cousin relationships of various sorts). So whatever spec feedback you send should ask for that to be added.

And one more thing. The spec feedback should go to the whatwg list, most likely.

(In reply to Boris Zbarsky (:bz) from comment #4) Thanks for getting back to me so quickly. > > I wonder if it would be useful to ask for a clarification to be added to section 5.1.4, > > Yes! There's a nice non-normative table at > http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers. > html#browsing-context-names but I see nothing in the normative text that > would give the behavior in that table. So the spec definitely needs fixing > here. Yes, it does explain it in a way that matches with the table later on, but this text should be clearer or maybe link to the sandboxing section for more information. > In terms of what the right solution is.... Given the desired behavior it > seems like the right thing is to either check in FindItemWithName in the > null aRequestor case (possibly by creating a DoFindItemWithName that does > all the work and then checking the return value in FindItemWithName) or to > check in the caller(s) of FindItemWithName. There aren't that many of > those, actually. But I'd prefer the "check in FindItemWithName if > aRequestor is null" approach. Ah yes, moving the current functionality into another function is clearly a cleaner way of doing it, thanks. I'll look into that. (In reply to Boris Zbarsky (:bz) from comment #5) > And one other note. The spec's table misses a case: "name that exists and > is a descendant of some ancestor but not an ancestor or descendant of > current" (basically uncle/aunt/nephew/niece/cousin relationships of various > sorts). So whatever spec feedback you send should ask for that to be added. Yes, this covers case 1 in the bug description as well, I'd noticed that it wasn't covered in that table. I'll mention this in the feedback to the spec. I'll also add tests for the other cases for named navigation in that table. (In reply to Boris Zbarsky (:bz) from comment #6) > And one more thing. The spec feedback should go to the whatwg list, most > likely. Is this whatwg@whatwg.org ? The spec also mentions commenting using their Bugzilla database, should I do both? Thanks, Bob

> Is this whatwg@whatwg.org ? Yep. > The spec also mentions commenting using their Bugzilla database, should I do both? Can't hurt. One option is to file the bug, then post a summary to the list with a link to the bug. The fundamental problem is that Hixie is more likely to change this part of the spec than the W3C editors are. So we want to make sure he sees this, and while he watches whatwg bugs he doesn't watch HTML5 bugs... or something. So you probably want to file at https://www.w3.org/Bugs/Public/enter_bug.cgi?product=WHATWG&component=HTML

I've written more tests covering extra cases and started on the fix. Just done the refactoring with the non-null aRequestor part in another function first, to make sure it is still working in the same way before I make further changes. Can't work on the code at the moment as I'm away for the weekend. So, I've added a bug to the HTML spec (using the link bz provided) and emailed a summary to whatwg@whatwg.org. https://www.w3.org/Bugs/Public/show_bug.cgi?id=20939 bz - I hope you don't mind, I've mentioned that I discussed this with you in the email. Thought someone might take more notice, rather than just an email from somebody of whom they've never heard. Cheers, Bob

Added extra tests to cater for other named navigation scenarios from non-normative table at: http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#browsing-context-names Fixed a couple of tests that currently would never fail. Changed the way negative tests work, mainly for the ones that run in their own window, as these were sometimes being closed before they had had a chance to fail. Kept tests separate from code, so the bug can be demonstrated first before applying the fix.

Moved the search for an item by actual name (i.e. not _top, _self etc.) to new function DoFindItemWithName. When aRequestor not null, then just hand off to DoFindItemWithName. When aRequestor is null, after special keyword checks call DoFindItemWithName, so that any found item gets sandbox checking if required. Had to change sandbox checks slightly, as they weren't quite working for items returned from DoFindItemWithName. A couple of extra questions: * Should I make DoFindItemWithName private? * In my change to the sandbox checks I'm passing an nsCOMPtr back into one of its getters e.g.: parentAsItem->GetSameTypeParent(getter_AddRefs(parentAsItem)); Seems to be working, but not sure if this will cause problems. I would like to do some more work on FindItemWithName, but I'm going to have to change it again for bug 766282, so I'll do it there. Cheers, Bob

Comment on attachment 712895 [details] [diff] [review] Named Sandboxed Navigation Bug Tests Review of attachment 712895 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks good to me. A few small nits. Also, maybe a little more clarification around a test or two. I like using the windowsToClose approach, it seems more reliable than what I was doing previously. A setTimeout() is often a red flag in tests but I'm not sure what else to do in this situation. ::: content/html/content/test/test_iframe_sandbox_navigation.html @@ +31,5 @@ > function receiveMessage(event) { > // this message is part of if_10's test > if (event.data.test == 'if_10') { > doIf10TestPart2(); > + nit: extra blank line @@ +63,5 @@ > +function testAttempted() { > + attemptedTests++; > + if (attemptedTests == totalTestsToAttempt) { > + // Make sure all tests have had a chance to complete. > + setTimeout(function() {finish();}, 1000); in general, doing this sort of thing can lead to intermittent test failures (oranges), but I'm not sure what other approach we can take here - we know we had a race-y test problem before your fixes here already. We will see if the reviewer(s) have any advice :) @@ +76,5 @@ > + is(passedTests, totalTestsToPass, "There are " + totalTestsToPass + " navigation tests that should pass"); > + > + for (var i = 0; i < windowsToClose.length; i++) { > + windowsToClose[i].close(); > + } doing the closing this way definitely seems more reliable to me. @@ +77,5 @@ > + > + for (var i = 0; i < windowsToClose.length; i++) { > + windowsToClose[i].close(); > + } > + nit: extra whitespace @@ +138,2 @@ > window.open("file_iframe_sandbox_d_if13.html"); > extra whitespace, but it was already here.. @@ +164,5 @@ > // file_iframe_sandbox_e_if5.html which navigates top > window.open("file_iframe_sandbox_e_if4.html"); > + > + // fails if bad > + // 14) iframe with sandbox='allow-same-origin allow-scripts allow-top-navigation' should not nit: trailing whitespace @@ +165,5 @@ > window.open("file_iframe_sandbox_e_if4.html"); > + > + // fails if bad > + // 14) iframe with sandbox='allow-same-origin allow-scripts allow-top-navigation' should not > + // be able to navigate another window (opened by another browsing context) using its name. only if it's not a descendant of the sandboxed iframe, right ? This is a little confusing. If the window opened is top level, it should be able to be navigated using its name since the iframe has allow-top-navigation, no? @@ +170,5 @@ > + // file_iframe_sandbox_d_if14.html in if_14 attempts to navigate "window_to_navigate", > + // which has been opened in preparation. > + > + // fails if bad > + // 15) iframe with sandox='allow-scripts' should not be able to navigate top using its name typo: sandox -> sandbox @@ +173,5 @@ > + // fails if bad > + // 15) iframe with sandox='allow-scripts' should not be able to navigate top using its name > + // as allow-top-navigation is not specified. > + // file_iframe_sandbox_e_if7.html contains file_iframe_sandbox_e_if8.html, which > + // attempts to navigate top by name. you might want to specify here that this is testing using top's real name and not '_top' @@ +177,5 @@ > + // attempts to navigate top by name. > + windowsToClose.push(window.open("file_iframe_sandbox_e_if7.html")); > + > + // fails if bad > + // 16) iframe with sandbox='allow-same-origin allow-scripts allow-top-navigation' should not nit: trailing whitespace @@ +179,5 @@ > + > + // fails if bad > + // 16) iframe with sandbox='allow-same-origin allow-scripts allow-top-navigation' should not > + // be able to use its parent's name to navigate it, when it is not top. > + // (Note: this would apply to other ancestor's that are not top as well.) nit: extra ' in ancestors

(In reply to Bob Owen from comment #11) > Created attachment 712902 [details] [diff] [review] > Named Sandboxed Navigation fix to nsDocShell > > Moved the search for an item by actual name (i.e. not _top, _self etc.) to > new function DoFindItemWithName. > > When aRequestor not null, then just hand off to DoFindItemWithName. > > When aRequestor is null, after special keyword checks call > DoFindItemWithName, so that any found item gets sandbox checking if required. > > Had to change sandbox checks slightly, as they weren't quite working for > items returned from DoFindItemWithName. overall, this looks good to me based on the discussion in the bug, but I will largely defer to the reviewer on this one. > A couple of extra questions: > > * Should I make DoFindItemWithName private? makes sense to me. > * In my change to the sandbox checks I'm passing an nsCOMPtr back into one > of its getters e.g.: > > parentAsItem->GetSameTypeParent(getter_AddRefs(parentAsItem)); > > Seems to be working, but not sure if this will cause problems. this seems a little sketchy to me (although it's working) but again i'll defer to the reviewer here. I'm paranoid about such things in general :) > I would like to do some more work on FindItemWithName, but I'm going to have > to change it again for bug 766282, so I'll do it there. sounds good to me. If you're happy with this patch and the tests, i'd suggest flagging them to bz for review (and maybe using your new level 1 access to push them both to try :) )

(In reply to Ian Melven :imelven from comment #12) > Comment on attachment 712895 [details] [diff] [review] > Named Sandboxed Navigation Bug Tests > Overall this looks good to me. A few small nits. Also, maybe a little more > clarification around a test or two. > I like using the windowsToClose approach, it seems more reliable than what I > was doing previously. Thanks. > ::: content/html/content/test/test_iframe_sandbox_navigation.html > @@ +31,5 @@ > > function receiveMessage(event) { > > // this message is part of if_10's test > > if (event.data.test == 'if_10') { > > doIf10TestPart2(); > > + > > nit: extra blank line I was trying to make it easier to read, but I think you're right, it looks better without it. > > @@ +63,5 @@ > > +function testAttempted() { > > + attemptedTests++; > > + if (attemptedTests == totalTestsToAttempt) { > > + // Make sure all tests have had a chance to complete. > > + setTimeout(function() {finish();}, 1000); > > in general, doing this sort of thing can lead to intermittent test failures > (oranges), but I'm not sure what other approach we can take here - we know > we had a race-y test problem before your fixes here already. We will see if > the reviewer(s) have any advice :) It shouldn't really lead to intermittent test failures, at worst it might mean if errors are introduced they won't be picked up. I'm just about to post another patch where I've added new tests using window.open() with a target, for all of the current tests that use the target attribute on an anchor. These throw an error when they are blocked, so we can post a passed test. This improves the coverage generally, but also it uses much of the same search code, so should help to catch any errors that might be introduced in the future. > @@ +77,5 @@ > > + > > + for (var i = 0; i < windowsToClose.length; i++) { > > + windowsToClose[i].close(); > > + } > > + > > nit: extra whitespace Thanks. Couldn't work out how I'd missed all this white space again. Then I realised that this file needs dos2unix and my search wasn't allowing for the ^Ms in the diff file. I've created a vim macro so I can easily check the files as I go now. It allow for the ^M, so I can use it on the diff file as well. I was going to leave the dos2unix until bug 766282, but I think I post a separate patch with it done for this one. > @@ +165,5 @@ > > window.open("file_iframe_sandbox_e_if4.html"); > > + > > + // fails if bad > > + // 14) iframe with sandbox='allow-same-origin allow-scripts allow-top-navigation' should not > > + // be able to navigate another window (opened by another browsing context) using its name. > > only if it's not a descendant of the sandboxed iframe, right ? It can't be our descendant, as it wasn't opened by us. Actually I don't think it counts as a child even if it is opened by us. > This is a little confusing. If the window opened is top level, it should be > able to be navigated using its name since the iframe has > allow-top-navigation, no? I think we should only be able to navigate our own top. > @@ +170,5 @@ > > + // file_iframe_sandbox_d_if14.html in if_14 attempts to navigate "window_to_navigate", > > + // which has been opened in preparation. > > + > > + // fails if bad > > + // 15) iframe with sandox='allow-scripts' should not be able to navigate top using its name > > typo: sandox -> sandbox Thanks ... I've lost count the number of times I've typed sanbox or sandox, my fingers seem to have mis-learned it :) > @@ +173,5 @@ > > + // fails if bad > > + // 15) iframe with sandox='allow-scripts' should not be able to navigate top using its name > > + // as allow-top-navigation is not specified. > > + // file_iframe_sandbox_e_if7.html contains file_iframe_sandbox_e_if8.html, which > > + // attempts to navigate top by name. > > you might want to specify here that this is testing using top's real name > and not '_top' Yes, just to make it clear. > @@ +179,5 @@ > > + > > + // fails if bad > > + // 16) iframe with sandbox='allow-same-origin allow-scripts allow-top-navigation' should not > > + // be able to use its parent's name to navigate it, when it is not top. > > + // (Note: this would apply to other ancestor's that are not top as well.) > > nit: extra ' in ancestors Thanks, I think I had "ancestor's names" at one point, but that would have still been wrong - the apostrophe should have been after the s :) (In reply to Ian Melven :imelven from comment #13) > (In reply to Bob Owen from comment #11) > > Created attachment 712902 [details] [diff] [review] > > Named Sandboxed Navigation fix to nsDocShell > overall, this looks good to me based on the discussion in the bug, but I > will largely defer to the reviewer on this one. Thanks. > > > A couple of extra questions: > > > > * Should I make DoFindItemWithName private? > > makes sense to me. I'll change this. > > > * In my change to the sandbox checks I'm passing an nsCOMPtr back into one > > of its getters e.g.: > > > > parentAsItem->GetSameTypeParent(getter_AddRefs(parentAsItem)); > > > > Seems to be working, but not sure if this will cause problems. > > this seems a little sketchy to me (although it's working) but again i'll > defer to the reviewer here. I'm paranoid about such things in general :) I've had a look the code for nsCOMPtr and I don't pretend to understand all (or even much) of it yet, but it looks like it caters for this by saving the original reference as it assigns the new one and the releasing the old one. Not entirely sure its safe though, so we'll see what the reviewer says. > > I would like to do some more work on FindItemWithName, but I'm going to have > > to change it again for bug 766282, so I'll do it there. > > sounds good to me. If you're happy with this patch and the tests, i'd > suggest flagging them to bz for review (and maybe using your new level 1 > access to push them both to try :) ) I'll make those changes and post the new patch with the extra tests as well.

Dealt with issues raised in Comment 12. Added navigation by window.open(..., <target>) tests to enhance the existing navigation by anchor target attribute tests. I think I've found a different problem with the way that window.open() works when you give it an existing target. If you target a top-level browsing context (either by _top or its name), then the browsing context that is doing the navigating replaces the existing opener ... even if it is a child of top. This doesn't seem right. The spec suggests that opener should be set once and not changed, unless it is to null. The problem appears to be in nsWindowWatcher::ReadyOpenedDocShellItem, where it sets the opener, even if it isn't a new window. This is called by nsWindowWatcher::OpenWindowInternal. I've found an old bug 174349 that seems to be for the same thing.

Just used dos2unix on some of the existing test files. See previous attachment 713432 [details] [diff] [review], for the detailed diff.

Made nsDocShell::DoFindItemWithName private.

Further to the possible bug where window.opener is replaced when window.open() is used on an existing target (see Comment 15) ... Opera and IE, don't replace the opener, Chrome does.

Removed tests that highlight the window.opener / window.open() problem (see Comment 15), so these don't cause failures on the try server. These should probably go in a different bug anyway, if it is decided that the behaviour is incorrect.

Discovered that parentAsItem->GetSameTypeParent(getter_AddRefs(parentAsItem)); isn't safe. The code I read was for assignment, where nsCOMPtr is in control of the whole operation. With getter_AddRefs() it calls that assignment code with null (0), which Releases and clears out the old internal pointer first, so this could definitely cause problems. After reading https://developer.mozilla.org/en-US/docs/Using_nsCOMPtr/Reference_Manual#Iterating, and other parts of the manual, I've changed it (hopefully) to the recommended pattern. Cheers, Bob

Changed foolish uses of window.top back to window.parent. They caused a time-out on the try server as they fail when run as part of a suite. Also, added a check on unload to see if the local finish() function has been called. If it hasn't then close any remaining windows, to try and prevent problems with later tests. I've pushed these for another try run (https://tbpl.mozilla.org/?tree=Try&rev=e2f1fedcdce3) and there are failures for some xpcshell tests, which is a bit strange as these passed last time, although they did run on a different platform. I won't be able to look at these until Monday now.

Had a failing xpcshell test (security/manager/ssl/tests/unit/test_signed_apps.js) on the last try run. The test did seem to be failing locally, although I could not see how it related to any of my changes. Re-pulled from mozilla-central and reapplied my changes and now the test passes, so I have pushed to try again (https://tbpl.mozilla.org/?tree=Try&rev=68d9e3942d30).

While working on bug 766282, I realised that I was wrong to remove the CanAccessItem check when the named item being looked for is us. Removing it might cause the search to stop too early with an item that subsequently turned out to be inaccessible.

Looks like some assertions in m-1 in the try run causing a failure - this may be fallout from https://groups.google.com/forum/#!msg/mozilla.dev.planning/AkJjs-FxYeo/mXgN01mJSzUJ

Yes, sorry had the following in my Additional Comments box, I'd obviously not clicked "Save Changes" ... Try run failed this time as I had missed the addition of the line SimpleTest.expectAssertions(1, 2); in test_iframe_sandbox_navigation.html. I've run dos2unix over this, so I didn't spot the change. Everything else passed this time so here's another attempt (https://tbpl.mozilla.org/?tree=Try&rev=901a111d6efb)

(In reply to Bob Owen from comment #25) > > Everything else passed this time so here's another attempt > (https://tbpl.mozilla.org/?tree=Try&rev=901a111d6efb) looking mighty green ! i'm looking over the patches right now :D

Comment on attachment 718537 [details] [diff] [review] Named Sandboxed Navigation Bug Tests Review of attachment 718537 [details] [diff] [review]: ----------------------------------------------------------------- Looking good ! ::: content/html/content/test/test_iframe_sandbox_navigation.html @@ -1,4 @@ > -<!DOCTYPE HTML> > -<html> > -<!-- > -https://bugzilla.mozilla.org/show_bug.cgi?id=341604 splinter got confused with this file, line ending issues ? it looks quite odd in the patch as well. The new window.open tests looks good. They look quite thorough to me, thank you :)

Comment on attachment 722755 [details] [diff] [review] Named Sandboxed Navigation fix to nsDocShell Review of attachment 722755 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thanks for linking to the nsCOMPtr reference manual, that was a good tidbit to read about :)

Comment on attachment 718537 [details] [diff] [review] Named Sandboxed Navigation Bug Tests Review of attachment 718537 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Ian. ::: content/html/content/test/test_iframe_sandbox_navigation.html @@ -1,4 @@ > -<!DOCTYPE HTML> > -<html> > -<!-- > -https://bugzilla.mozilla.org/show_bug.cgi?id=341604 I ran dos2unix over it, so I suppose that might have been it.

jst asked me to have a look here. Docshell is in general a tangled, scary mess; thanks a lot for helping with it! Ian, I'm happy to let your f+ on the tests be a r+, if you're comfortable with that. Let me know if there's anything in particular you want me to look at there. > * Should I make DoFindItemWithName private? Yes, please. >diff --git a/docshell/base/nsDocShell.h b/docshell/base/nsDocShell.h >--- a/docshell/base/nsDocShell.h >+++ b/docshell/base/nsDocShell.h >@@ -864,16 +864,23 @@ protected: > private: >+ // Separate function to do the actual name (i.e. not _top, _self etc.) >+ // searching for FindItemWithName. >+ nsresult DoFindItemWithName(const PRUnichar * aName, >+ nsISupports * aRequestor, >+ nsIDocShellTreeItem * aOriginalRequestor, >+ nsIDocShellTreeItem ** _retval); Nit: This header file has an embarrassing variety of "*" styles, but the preferred one these days is |Foo* aBar|, and since this file has no prevaling style, I'd prefer we used the preferred one. >diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp >--- a/docshell/base/nsDocShell.cpp >+++ b/docshell/base/nsDocShell.cpp >@@ -3166,24 +3166,28 @@ nsDocShell::FindItemWithName(const PRUni > NS_ENSURE_ARG_POINTER(_retval); > > // If we don't find one, we return NS_OK and a null result > *_retval = nullptr; > > if (!*aName) > return NS_OK; > >- if (!aRequestor) >- { >- nsCOMPtr<nsIDocShellTreeItem> foundItem; >+ if (aRequestor) { >+ // We are being called as part of the recursive search just hand >+ // straight off to the search by actual name function. >+ return DoFindItemWithName(aName, aRequestor, aOriginalRequestor, >+ _retval); Nit: This is a run-on sentence. Please add a semicolon, period, colon, or dash ("--", not "-") between "search" and "just". >@@ -3252,28 +3259,26 @@ nsDocShell::FindItemWithName(const PRUni > GetSameTypeRootTreeItem(getter_AddRefs(root)); > > // Is the found item not a top level browsing context and not ourself ? > nsCOMPtr<nsIDocShellTreeItem> selfAsItem = static_cast<nsIDocShellTreeItem *>(this); > if (foundItem != root && foundItem != selfAsItem) { Call this line (*); I'll refer to it later. > // Are we an ancestor of the foundItem ? > bool isAncestor = false; > >- nsCOMPtr<nsIDocShellTreeItem> parentAsItem; >- GetSameTypeParent(getter_AddRefs(parentAsItem)); Just to make sure I understand correctly, this code here was just wrong? It looks pretty wrong to me, because unless there's a cycle in the docshell ancestry graph, this->GetSameTypeParent() will never have an ancestor which equals |this|. >+ nsIDocShellTreeItem* tmp; >+ foundItem->GetSameTypeParent(&tmp); >+ nsCOMPtr<nsIDocShellTreeItem> parentAsItem = dont_AddRef(tmp); Nit: I'd prefer if tmp were an nsCOMPtr and we didn't deal with the dont_AddRef business. It means extra addref's and release's, but there's less room for error. (Even if this patch is right, we want to reduce the chance of our messing up in the future.) > while (parentAsItem) { >- nsCOMPtr<nsIDocShellTreeItem> tmp; >- parentAsItem->GetParent(getter_AddRefs(tmp)); >- >- if (tmp && tmp == selfAsItem) { >+ if (parentAsItem == selfAsItem) { > isAncestor = true; > break; > } >- parentAsItem = tmp; >+ parentAsItem->GetSameTypeParent(&tmp); >+ parentAsItem = dont_AddRef(tmp); > } > > if (!isAncestor) { > // No, we are not an ancestor and our document is > // sandboxed, we can't allow this. > foundItem = nullptr; > } > } else { AFAICT this code, in both its current and patched form, always allows a sandboxed frame to navigate its parent. That's because if foundItem == selfAsItem, the if statement at (*) is false. So we drop down to the else, which does the following: > // Top level browsing context - is it an ancestor of ours ? > nsCOMPtr<nsIDocShellTreeItem> tmp; > GetSameTypeParent(getter_AddRefs(tmp)); > > while (tmp) { > if (tmp && tmp == foundItem) { > // This is an ancestor, and we are sandboxed. > // Unless allow-top-navigation is set, we can't allow this. > if (sandboxFlags & SANDBOXED_TOPLEVEL_NAVIGATION) { > foundItem = nullptr; > } > break; > } > tmp->GetParent(getter_AddRefs(tmp)); > } This will never do |foundItem = nullptr| if foundItem == this (where this == selfAsItem) because it starts iterating at this->GetSameTypeParent(). From reading the spec, it doesn't seem like that's the desired behavior. I'm not sure that's the only bug here, but I hope you don't mind if I take this one step at a time. FYI I'll be on vacation for the next week, so sorry in advance if I'm slow getting back to you here.

(In reply to Justin Lebar [:jlebar] (PTO 3/11-3/15, then GMT+9 until 3/24) from comment #30) > jst asked me to have a look here. Thanks. > Docshell is in general a tangled, scary mess; thanks a lot for helping with > it! No problem, I have had to chase around quite a lot of code to work out what is going on. > >diff --git a/docshell/base/nsDocShell.h b/docshell/base/nsDocShell.h > >--- a/docshell/base/nsDocShell.h > >+++ b/docshell/base/nsDocShell.h > > >@@ -864,16 +864,23 @@ protected: > > private: > >+ // Separate function to do the actual name (i.e. not _top, _self etc.) > >+ // searching for FindItemWithName. > >+ nsresult DoFindItemWithName(const PRUnichar * aName, > >+ nsISupports * aRequestor, > >+ nsIDocShellTreeItem * aOriginalRequestor, > >+ nsIDocShellTreeItem ** _retval); > > Nit: This header file has an embarrassing variety of "*" styles, but the > preferred one these days is |Foo* aBar|, and since this file has no prevaling > style, I'd prefer we used the preferred one. OK no problem, I was just keeping the same style as FindItemWithName, but that is the style I would normally use. > >+ if (aRequestor) { > >+ // We are being called as part of the recursive search just hand > >+ // straight off to the search by actual name function. > >+ return DoFindItemWithName(aName, aRequestor, aOriginalRequestor, > >+ _retval); > > Nit: This is a run-on sentence. Please add a semicolon, period, colon, or > dash > ("--", not "-") between "search" and "just". Oh dear, it doesn't read very well does it. How about ... // We are being called as part of the recursive search, so just // hand straight off to the search by actual name function. > >- nsCOMPtr<nsIDocShellTreeItem> parentAsItem; > >- GetSameTypeParent(getter_AddRefs(parentAsItem)); > > Just to make sure I understand correctly, this code here was just wrong? It > looks pretty wrong to me, because unless there's a cycle in the docshell > ancestry graph, this->GetSameTypeParent() will never have an ancestor which > equals |this|. No, it would never have worked. However, because the sandbox checks were only being done after the special cases (_top, _parent, etc.), we could never have been an ancestor of |foundItem| at this point anyway. > >+ nsIDocShellTreeItem* tmp; > >+ foundItem->GetSameTypeParent(&tmp); > >+ nsCOMPtr<nsIDocShellTreeItem> parentAsItem = dont_AddRef(tmp); > > Nit: I'd prefer if tmp were an nsCOMPtr and we didn't deal with the > dont_AddRef > business. It means extra addref's and release's, but there's less room for > error. (Even if this patch is right, we want to reduce the chance of our > messing up in the future.) OK, I'll change it back. I was just following the recommended pattern from the reference manual. We're not likely to be looping for long, so performance wise it probably makes little difference. > > while (parentAsItem) { > >- nsCOMPtr<nsIDocShellTreeItem> tmp; > >- parentAsItem->GetParent(getter_AddRefs(tmp)); > >- > >- if (tmp && tmp == selfAsItem) { > >+ if (parentAsItem == selfAsItem) { > > isAncestor = true; > > break; > > } > >- parentAsItem = tmp; > >+ parentAsItem->GetSameTypeParent(&tmp); > >+ parentAsItem = dont_AddRef(tmp); > > } > > > > if (!isAncestor) { > > // No, we are not an ancestor and our document is > > // sandboxed, we can't allow this. > > foundItem = nullptr; > > } > > } else { > > AFAICT this code, in both its current and patched form, always allows a > sandboxed frame to navigate its parent. > > That's because if foundItem == selfAsItem, the if statement at (*) is false. I'm not sure I quite understand what you mean here. Do you mean a frame or an iframe? Also, if it was navigating its parent then wouldn't foundItem not be equal to selfAsItem. > So we drop down to the else, which does the following: > > > // Top level browsing context - is it an ancestor of ours ? > > nsCOMPtr<nsIDocShellTreeItem> tmp; > > GetSameTypeParent(getter_AddRefs(tmp)); > > > > while (tmp) { > > if (tmp && tmp == foundItem) { > > // This is an ancestor, and we are sandboxed. > > // Unless allow-top-navigation is set, we can't allow this. > > if (sandboxFlags & SANDBOXED_TOPLEVEL_NAVIGATION) { > > foundItem = nullptr; > > } > > break; > > } > > tmp->GetParent(getter_AddRefs(tmp)); > > } > > This will never do |foundItem = nullptr| if foundItem == this (where this == > selfAsItem) because it starts iterating at this->GetSameTypeParent(). > > From reading the spec, it doesn't seem like that's the desired behavior. > > I'm not sure that's the only bug here, but I hope you don't mind if I take > this > one step at a time. If foundItem == this, aren't we always allowed to navigate it? I've actually refactored the sandboxing checks in bug 766282, which was what I was working on when I noticed this bug. 766282 is to add allow-popups to the sandbox keywords and I'm having to change the sandbox checks to allow the "one permitted sandboxed navigator". So, I thought I'd put all the changes in one place and try and keep the fix here fairly simple. I'll hopefully be posting the patch for 766282 fairly soon. > FYI I'll be on vacation for the next week, so sorry in advance if I'm slow > getting back to you here. I hope you have (have had) a great holiday. I've got an assignment for my Maths MSc to finish by Wednesday and I'm hoping to start a new contract on Thursday, so I might not get these changes done until next weekend anyway.

I recently talked with Hixie about some of this stuff. He says that this kind of issue is one of the main reasons why the HTML5 spec describes sandboxing happening at load time rather than implementing a simple property filter for stuff like Window.location. He strongly suggests that we implement sandbox restrictions as the spec suggests, and I think I agree here. So for both this bug and bug 785310, I think we'll want to do a dynamic heck during loading and determine if the script entry point is sandboxed. If so, we'll want to do the relevant computation to determine if it's allowed.

(In reply to Justin Lebar [:jlebar] (PTO 3/11-3/15, then GMT+9 until 3/24) from comment #30) > > Ian, I'm happy to let your f+ on the tests be a r+, if you're comfortable > with > that. Let me know if there's anything in particular you want me to look at > there. Thanks for taking a look at all this, Justin. I'm fine with you making the f+ on the tests an r+ - the most important thing to look at probably is the numbered list of test cases in test_iframe_sandbox_navigation.html to see if you can think of any additional interesting cases we should be testing.

Address issues in Comment 30 from Justin. * Formatting for DoFindItemWithName in declaration and definition. * Comment readability in FindItemWithName aRequestor not null case. * Use nsCOMPtr, instead of temporary pointer with dont_AddRefs, in FindItemWithName sandbox ancestor checking.

> So for both this bug and bug 785310, I think we'll want to do a dynamic heck during > loading and determine if the script entry point is sandboxed. I think that's fine, but do you mind if we do it in a separate bug? I'd rather not get derailed here, and we'll very likely use the code this patch is touching in order to implement such a check.

(In reply to Justin Lebar [:jlebar] from comment #35) > > So for both this bug and bug 785310, I think we'll want to do a dynamic heck during > > loading and determine if the script entry point is sandboxed. > > I think that's fine, but do you mind if we do it in a separate bug? Can I second that and also ask the same goes for bug 766282. The code there will also probably stay the same either way. I've just posted a new patch for that bug as well. Cheers, Bob

(In reply to Bob Owen from comment #36) > (In reply to Justin Lebar [:jlebar] from comment #35) > > > So for both this bug and bug 785310, I think we'll want to do a dynamic heck during > > > loading and determine if the script entry point is sandboxed. > > > > I think that's fine, but do you mind if we do it in a separate bug? > > Can I second that and also ask the same goes for bug 766282. > The code there will also probably stay the same either way. > I've just posted a new patch for that bug as well. > > Cheers, > Bob I think that 785310 and 766282 are the iframe sandbox bugs that involve script (setting window.location and window.open, respectively). Bug 766282 will touch nsDocShell::FindItemWithName as well, but this bug (and Bob's patch) is mostly about addressing the <a target='some named frame that isn't top'> cases with iframe sandbox (target='top' was blocked in the original implementation, unless the iframe has 'allow-top-navigation). We could split the 'correct sandboxing based on script entry point happening at load time' work off into a separate bug (that I intend to work on) and then make 785310 and 766282 dependent on that - is that reasonable ?

I'm really sorry I've been so unresponsive here. I've been crushed under the weight of B2G lately, but that's not an excuse. At the very least: I haven't forgotten about this.

(In reply to Justin Lebar [:jlebar] from comment #38) > I'm really sorry I've been so unresponsive here. I've been crushed under > the weight of B2G lately, but that's not an excuse. At the very least: I > haven't forgotten about this. No worries, I fell down an B2G emulator/CSP hole for a couple weeks myself, which is why I've been lagging on iframe sandbox as well :(

> We could split the 'correct sandboxing based on script entry point happening at load time' work off > into a separate bug (that I intend to work on) and then make 785310 and 766282 dependent on that - > is that reasonable ? Go for it.

Hey jlebar - I see in comment 38 you're pretty slammed, but do you know when you'll have time to review this (or is there someone else you can recommend)?

bz can certainly review this, if he has time. Smaug might also feel comfortable doing so. I'm stuck with some tef+ bugs that are unraveling themselves into increasingly large issues.

Comment on attachment 725773 [details] [diff] [review] Named Sandboxed Navigation fix to nsDocShell smaug reviewed the original iframe sandbox work so that makes sense, sorry Olli ;)

Sorry about delay. Review coming today.

Comment on attachment 725773 [details] [diff] [review] Named Sandboxed Navigation fix to nsDocShell >- if (!aRequestor) >- { >- nsCOMPtr<nsIDocShellTreeItem> foundItem; >+ if (aRequestor) { >+ // We are being called as part of the recursive search, so just hand >+ // straight off to the search by actual name function. This is a bit wrong. non-null aRequestor doesn't necessarily mean recursive call, kind of. The API is really odd. aRequestor parameter really should be DocShell internal thing, not exposed in nsIDocShell. But just fix the comment.

(In reply to Olli Pettay [:smaug] from comment #45) > Comment on attachment 725773 [details] [diff] [review] > Named Sandboxed Navigation fix to nsDocShell > > > >- if (!aRequestor) > >- { > >- nsCOMPtr<nsIDocShellTreeItem> foundItem; > >+ if (aRequestor) { > >+ // We are being called as part of the recursive search, so just hand > >+ // straight off to the search by actual name function. > This is a bit wrong. non-null aRequestor doesn't necessarily mean recursive > call, kind of. > The API is really odd. aRequestor parameter really should be DocShell > internal thing, not exposed in nsIDocShell. > But just fix the comment. Thanks Olli, I'll change that comment. Just got to wait for my compile to finish, as I've pulled the latest changes. Not sure it will be tonight, so I'll hopefully get this posted tomorrow night. Do I add you as a reviewer again? If I have time I could look at making further changes in my patch for bug 766282. This was the bug that I was originally working on when I found this problem. With the new DoFindItemWithName function, it should be fairly straight forward to remove the aRequestor from FindItemWithName (and all the callers that pass null). Then change all the callers that pass aRequestor (which should all be in nsDocShell) to call DoFindItemWithName directly, which is effectively what happens anyway. I'm already changing most (if not all) of the files involved in that bug anyway and have done some other refactoring for the sandboxing checks as well. Cheers, Bob

(In reply to Bob Owen from comment #46) > > Thanks Olli, I'll change that comment. > Just got to wait for my compile to finish, as I've pulled the latest changes. > Not sure it will be tonight, so I'll hopefully get this posted tomorrow > night. > Do I add you as a reviewer again? Olli marked it r+ so it should be fine to carry over the r+ when you fix the comment :) > If I have time I could look at making further changes in my patch for bug > 766282. > This was the bug that I was originally working on when I found this problem. > With the new DoFindItemWithName function, it should be fairly straight > forward to remove the aRequestor from FindItemWithName (and all the callers > that pass null). > Then change all the callers that pass aRequestor (which should all be in > nsDocShell) to call DoFindItemWithName directly, which is effectively what > happens anyway. > I'm already changing most (if not all) of the files involved in that bug > anyway and have done some other refactoring for the sandboxing checks as > well. my understanding is that the patch in this bug fixes some issues with the existing implementation, which is why I was pushing to land this asap while we figure out the scripted loads stuff (including bug 766282's window.open/allow-popups functionality) which may take longer - is that right ? I'd suggest maybe splitting the refactoring/improvements you suggest around FindItemWithName/DoFindItemWithName into a separate bug rather than bundling them with allow-popups, but of course it's up to you :) Also, I know your time is limited at the moment, Bob. Thank you again for all your hard work on this stuff and these patches. I plan to review the tests this week and then land this bug :D

(In reply to Ian Melven :imelven from comment #47) > (In reply to Bob Owen from comment #46) > my understanding is that the patch in this bug fixes some issues with the > existing implementation, which is why I was pushing to land this asap while > we figure out the scripted loads stuff (including bug 766282's > window.open/allow-popups functionality) which may take longer - is that > right ? I'd suggest maybe splitting the refactoring/improvements you suggest > around FindItemWithName/DoFindItemWithName into a separate bug rather than > bundling them with allow-popups, but of course it's up to you :) I think you're right that 766282 is going to take a while to land with the script related changes, so I'll raise this code improvement as a separate bug. > Also, I know your time is limited at the moment, Bob. Thank you again for > all your hard work on this stuff and these patches. I plan to review the > tests this week and then land this bug :D Thanks, it'll be good to get my first bug fix landed :)

Fixed comment in FindItemWithName as per review in Comment 45.

Comment on attachment 718537 [details] [diff] [review] Named Sandboxed Navigation Bug Tests Review of attachment 718537 [details] [diff] [review]: ----------------------------------------------------------------- Looks great overall. Some very small comments. I'd like to look more at how these window.open changes will interact with the scripting entry point stuff we've been discussing in bug 859454, I'll do that next. Thanks again for adding the try/catch bits and the tracking of attempted tests to make things more reliable. I don't think doing the reordering I've commented on is that important, but it might make things very slightly more bulletproof. The iframe sandbox navigation tests pass locally for me, I did a limited try run to see how that goes: https://tbpl.mozilla.org/?tree=Try&rev=66846a5871fa ::: content/html/content/test/file_iframe_sandbox_d_if16.html @@ +9,5 @@ > + > +<script type="application/javascript"> > +function doTest() { > + sendMouseEvent({type:'click'}, 'anchor'); > + window.parent.parent.postMessage("test attempted", "*"); nit: maybe reorder so the message always goes out first ? ::: content/html/content/test/file_iframe_sandbox_d_if20.html @@ +11,5 @@ > + > + function doTest() { > + try { > + window.open("file_iframe_sandbox_navigation_fail.html?" + escape(testContext), "_parent"); > + window.parent.parent.postMessage("test attempted", "*"); nit: maybe reorder as previous ::: content/html/content/test/file_iframe_sandbox_d_if22.html @@ +11,5 @@ > + > + function doTest() { > + try { > + window.open("file_iframe_sandbox_navigation_fail.html?" + escape(testContext), "if_parent2"); > + window.parent.parent.postMessage("test attempted", "*"); nit: possibly reorder ? ::: content/html/content/test/file_iframe_sandbox_d_if4.html @@ +8,5 @@ > </head> > <script type="application/javascript"> > function doTest() { > sendMouseEvent({type:'click'}, 'anchor'); > + window.parent.parent.postMessage("test attempted", "*"); nit: might want to reorder here to increase chances of message getting out before parent is navigated ? ::: content/html/content/test/file_iframe_sandbox_e_if11.html @@ +10,5 @@ > + var testContext = location.search.substring(1); > + try { > + var topsOpener = window.top.opener; > + window.open("file_iframe_sandbox_top_navigation_pass.html?" + testContext, "_top"); > + topsOpener.postMessage({ok: true, desc: unescape(testContext) + "top navigation should be allowed by a document sandboxed with 'allow-top-navigation."}, "*"); same nit about reordering ::: content/html/content/test/file_iframe_sandbox_e_if14.html @@ +10,5 @@ > + var testContext = location.search.substring(1); > + try { > + var topsOpener = window.top.opener; > + window.open("file_iframe_sandbox_top_navigation_fail.html?" + testContext, "_top"); > + topsOpener.postMessage({ok: false, desc: unescape(testContext) + "top navigation should NOT be allowed by a document sandboxed without 'allow-top-navigation."}, "*"); same nit on reordering ::: content/html/content/test/file_iframe_sandbox_e_if16.html @@ +12,5 @@ > + function doTest() { > + try { > + var topsOpener = window.top.opener; > + window.open("file_iframe_sandbox_top_navigation_fail.html?" + escape(testContext), "e_if15"); > + topsOpener.postMessage({ok: false, desc: unescape(testContext) + "top navigation should NOT be allowed by a document sandboxed without 'allow-top-navigation."}, "*"); same nit on reordering ::: content/html/content/test/file_iframe_sandbox_e_if6.html @@ +10,5 @@ > +<script type="application/javascript"> > +function doTest() { > + document.getElementById('anchor').href = "file_iframe_sandbox_top_navigation_fail.html" + location.search; > + sendMouseEvent({type:'click'}, 'anchor'); > + window.top.opener.postMessage("test attempted", "*"); same nit about reordering ::: content/html/content/test/file_iframe_sandbox_e_if8.html @@ +10,5 @@ > +<script> > + function doTest() { > + // Try to navigate top using its name (e_if7). We should not be able to do this as allow-top-navigation is not specified. > + sendMouseEvent({type:'click'}, 'navigate_top'); > + window.top.opener.postMessage("test attempted", "*"); same nit about reordering ::: content/html/content/test/test_iframe_sandbox_navigation.html @@ -1,4 @@ > -<!DOCTYPE HTML> > -<html> > -<!-- > -https://bugzilla.mozilla.org/show_bug.cgi?id=341604 + // passes if good, fails if bad + // 21) iframe with sandbox='allow-same-origin allow-scripts allow-top-navigation' should not + // be able to use its parent's name (not _parent) to navigate it using window.open(), when it is not top. + // (Note: this would apply to other ancestors that are not top as well.) + // file_iframe_sandbox_d_if21.html in if_21 contains file_iframe_sandbox_d_if22.html, which + // tries to navigate if_21 by its name (if_parent2). should we add a version of this test using <a target= as well (mostly because of the allow-same-origin) ? a very small nit : + // 25) iframe with sandbox='allow-scripts' nested inside an iframe with + // 'allow-top-navigation allow-scripts' can NOT navigate top, with window.open(). + // file_iframe_sandbox_e_if13.html contains file_iframe_sandbox_e_if12.html which contains + // file_iframe_sandbox_e_if14.html which navigates top. this is using window.open(..., "_top") or so I infer from the next comment tiny nit: > // If our own finish() has not been called, probably failed due to a timeout, so close remaining windows. extra 'If'

From the try run : 17:15:22 INFO - 192598 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_iframe_sandbox_navigation.html | Assertion count 3 is greater than expected range 0-0 assertions.

(In reply to Ian Melven :imelven from comment #50) > Comment on attachment 718537 [details] [diff] [review] > Named Sandboxed Navigation Bug Tests > > Review of attachment 718537 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks great overall. Some very small comments. I'd like to look more at how > these window.open changes will interact with the scripting entry point > stuff we've been discussing in bug 859454, I'll do that next. Thanks. I've not attempted to address any of the new scripting entry point changes in this bug. I think the general feeling was that they would be picked up by other bugs and this fix would be needed either way. > Thanks again for adding the try/catch bits and the tracking of attempted > tests to make things more reliable. > > I don't think doing the reordering I've commented on is that important, but > it might make things very slightly more bulletproof. I'll look through all of the re-ordering points below. My general feeling is that it is better to call after the line that kicks off the test, as it gives that test the best chance to finish before everything is closed down. However, I take your point that when it is attempting to navigate one of its ancestor's then it might be better to do it before, just in case. > > ::: content/html/content/test/test_iframe_sandbox_navigation.html > @@ -1,4 @@ > > -<!DOCTYPE HTML> > > -<html> > > -<!-- > > -https://bugzilla.mozilla.org/show_bug.cgi?id=341604 > > + // passes if good, fails if bad > + // 21) iframe with sandbox='allow-same-origin allow-scripts > allow-top-navigation' should not > + // be able to use its parent's name (not _parent) to navigate it using > window.open(), when it is not top. > + // (Note: this would apply to other ancestors that are not top as well.) > + // file_iframe_sandbox_d_if21.html in if_21 contains > file_iframe_sandbox_d_if22.html, which > + // tries to navigate if_21 by its name (if_parent2). > > should we add a version of this test using <a target= as well (mostly > because of the allow-same-origin) ? I think this is covered in test 16. > a very small nit : > > + // 25) iframe with sandbox='allow-scripts' nested inside an iframe with > + // 'allow-top-navigation allow-scripts' can NOT navigate top, with > window.open(). > + // file_iframe_sandbox_e_if13.html contains > file_iframe_sandbox_e_if12.html which contains > + // file_iframe_sandbox_e_if14.html which navigates top. > > this is using window.open(..., "_top") or so I infer from the next comment Thanks, yes it is. I'll clarify this in the comment. > tiny nit: > > > // If our own finish() has not been called, probably failed due to a timeout, so close remaining windows. > > extra 'If' Not quite sure what you mean here. (In reply to Ian Melven :imelven from comment #51) > From the try run : 17:15:22 INFO - 192598 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/html/content/test/test_iframe_sandbox_navigation.html | > Assertion count 3 is greater than expected range 0-0 assertions. I saw that earlier today, I'll fix it. Cheers, Bob

I've been through the re-orderings you suggested and changed some of them. The ones with window.open() need to be after the statement, because they are actually posting whether the test was successful or not; with the opposite result being posted in the exception handler. Over the failure to do with the assertions in the try run, I've got the appropriate line in my local copy, so I must not have updated the patch. However, after the small changes I've made the tests are now failing locally, which is confusing, so it might take me a little while until I find enough time to work out what has gone wrong. Cheers, Bob

Had a bit of time this morning and, after a diff with the old patch, I soon found that I had accidentally put in a rouge line feed. It was right near where the line wrapped anyway, so I hadn't spotted it before. In response to review in Comment 50: Re-ordered some of the test lines. Clarified comment for test 25. In response to Comment 51 and try run: Re-added line to allow for assertions. Also added some missing single quotes into comments. Ian, I have carried over your r+ - I hope you are still OK with this. There are a couple of questions (in Comment 52) I had over your review. I've set another try run going with the same settings as the last one: https://tbpl.mozilla.org/?tree=Try&rev=5c4bcd4d0cb0 Cheers, Bob

(In reply to Bob Owen from comment #52) > (In reply to Ian Melven :imelven from comment #50) > > Comment on attachment 718537 [details] [diff] [review] > > Named Sandboxed Navigation Bug Tests > > > > Review of attachment 718537 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Looks great overall. Some very small comments. I'd like to look more at how > > these window.open changes will interact with the scripting entry point > > stuff we've been discussing in bug 859454, I'll do that next. > > Thanks. > I've not attempted to address any of the new scripting entry point changes > in this bug. I think the general feeling was that they would be picked up > by other bugs and this fix would be needed either way. yeah, I think that's the right approach. > > should we add a version of this test using <a target= as well (mostly > > because of the allow-same-origin) ? > > I think this is covered in test 16. ah yes, thanks > > tiny nit: > > > > > // If our own finish() has not been called, probably failed due to a timeout, so close remaining windows. > > > > extra 'If' > > Not quite sure what you mean here. seemed like a missing word or extra word in the comment, no worries :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/0200fd49efc3 https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b13623631c

the most recent try push : https://tbpl.mozilla.org/?tree=Try&rev=c1a26466ac96

Sorry, I backed this out on inbound because test_iframe_sandbox_navigation.html was timing out frequently on Linux debug builds: https://hg.mozilla.org/integration/mozilla-inbound/rev/6e52ff141274 https://tbpl.mozilla.org/php/getParsedLog.php?id=22746651&tree=Mozilla-Inbound

David Chan suggested to me that the timeouts may be due to the navigation tests having grown rather large (they doubled in count in this patch) and the test machines being slower than my laptop (where I cannot seem to reproduce the time out by running linux debug mochitest-1). Bob, does this split make sense to you ? Both halves pass for me locally. I'm pushing them to try for linux and linux64 mochitest-1 to see if we still see timeouts. If we don't see timeouts, I'll rerun them a few times to try and increase the certainty splitting the tests addresses the problem. If we still see timeouts, I'll keep digging. https://tbpl.mozilla.org/?tree=Try&rev=b5d48c3fde5f

Haven't had much time to look at them, but doing a diff with the old patch, it looks like a clean 50-50 split to me. Thanks! Just the SimpleTest.expectAssertions() that needs adjusting as you mentioned in your email. Back in work tomorrow, so I'll take a closer for the review when I get a chance. Cheers, Bob

Like Bob said, all the aassertions are in the first half of the tests :) https://tbpl.mozilla.org/?tree=Try&rev=154d572977b8

Did a bunch of mochitest-1 linux debug runs in the Try run in the previous comment and they look OK. Doing one last full try run before I try to land this again : https://tbpl.mozilla.org/?tree=Try&rev=f41ccd431787

Some named targets are allowed.

Comment on attachment 748064 [details] [diff] [review] Named Sandboxed Navigation Bug Tests v3 Review of attachment 748064 [details] [diff] [review]: ----------------------------------------------------------------- Finally had time to apply the new tests and run them locally. All green for me on a debug linux build. Thanks, Bob

Tests in content/html/content/test/test_iframe_sandbox_navigation2.html, were failing for B2G, we believe because of multiple concurrent uses of window.open(). Tried some quick changes to solve this, but I haven't had time to get it working. I was also wondering how the original tests (test_iframe_sandbox_navigation.html) were working. When I checked the b2g.json file they were already being skipped. The test_iframe_sandbox_navigation2.html file was only created because of timeouts on Linux debug builds, but of course these split out tests weren't being skipped in B2G. So, I have added test_iframe_sandbox_navigation2.html to b2g.json and pushed another full try https://tbpl.mozilla.org/?tree=Try&rev=1dd20794cc87 There are some things not working, but I think they are in areas where there are intermittent failures anyway. I can't retry them from this machine. I haven't uploaded the new tests patch yet, I'll do that when I get back from work this evening. I would like to get the tests working for B2G in the future, but I think that it is more important that I get this landed and then perhaps bug 766282 as well. If I look at these tests in the future I'll raise a new bug. Cheers, Bob

(In reply to Bob Owen from comment #65) > So, I have added test_iframe_sandbox_navigation2.html to b2g.json and pushed > another full try https://tbpl.mozilla.org/?tree=Try&rev=1dd20794cc87 > There are some things not working, but I think they are in areas where there > are intermittent failures anyway. > I can't retry them from this machine. I took a look at the Try run, starred a few things and reran a couple of others, I'll check the results later. > I haven't uploaded the new tests patch yet, I'll do that when I get back > from work this evening. if it's the one you pushed to Try, I can always grab it from there too in general :) > I would like to get the tests working for B2G in the future, but I think > that it is more important that I get this landed and then perhaps bug 766282 > as well. I agree and support that approach. > If I look at these tests in the future I'll raise a new bug. Makes sense to me. Thanks for pushing on this, Bob !

Added content/html/content/test/test_iframe_sandbox_navigation2.html to testing/mochitest/b2g.json, so that these tests are skipped for B2G like content/html/content/test/test_iframe_sandbox_navigation.html. This is the version of the tests in the try push in Comment 65.

pushed to inbound again, fingers crossed : https://hg.mozilla.org/integration/mozilla-inbound/rev/c873354a9242 https://hg.mozilla.org/integration/mozilla-inbound/rev/65cd5eac8b2e

https://hg.mozilla.org/mozilla-central/rev/c873354a9242 https://hg.mozilla.org/mozilla-central/rev/65cd5eac8b2e