Closed Bug 766282 Opened 12 years ago Closed 11 years ago

implement allow-popups directive for iframe sandbox

Categories

(Core :: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
relnote-firefox --- 27+

People

(Reporter: imelven, Assigned: bobowen)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: mentor=imelven lang=c++)

Attachments

(3 files, 17 obsolete files)

44.44 KB, patch
smaug
: review+
Details | Diff | Splinter Review
25.46 KB, patch
smaug
: review+
Details | Diff | Splinter Review
82.60 KB, patch
smaug
: review+
Details | Diff | Splinter Review
the allow-popups directive has made it into the W3C spec for iframe sandbox, see http://dev.w3.org/html5/spec/origin-0.html#attr-iframe-sandbox-allow-popups

we should consider implementing this directive - if there are security concerns about the way it's described in the spec, we should file these as spec bugs with the W3C 

i would like to land the existing implementation of iframe sandbox in bug 341604 and then discuss allow-popups and implement it as a followup, unless there are objections. Note that the implementation in 341604 _always_ blocks popups, with no opting out of this.
Depends on: framesandbox
I have come across this problem when using the new Office Mail Apps feature in Microsoft's Outlook Web App 2013 on Firefox (more information on this feature can be found at http://msdn.microsoft.com/en-us/library/jj220082.aspx)

OWA 2013 serves Office Mail Apps in a sandboxed Iframe with the 'allow-popups' directive. However in Firefox, when a link is clicked that should cause a new window to open, nothing happens - whereas this works on all other major browsers.

Are there any updates available on when Firefox will be updated to support the 'allow-popups' directive?
I have joined my voice to that ticket. This bug makes difficult to use iframe sandbox in popular web applications :(
I definitely believe we should implement allow-popups per the current WHATWG spec. Unfortunately, I have no plans to work on this bug in the immediate future due to other priorities. I would be very happy to help or mentor anyone else who wishes to work on it however !
Whiteboard: mentor=imelven lang=c++
Hi Ian,

I'd like to help with this if I can.
I'm new to the firefox codebase, so hopefully this isn't too complicated.

I've had a quick look.
I'm guessing I'll need to add another flag to content/base/src/nsSandboxFlags.h
Then modify ParseSandboxAttributeToFlags in content/base/src/nsContentUtils.cpp to parse and set this flag.

I'm also guessing this is the easy part.
What I don't really have any idea on yet, is where I need to check this flag and allow the creation of the popup.

Thanks,
Bob
Hi again Ian,

I've realised this morning that I can get a lot of the information from reading through your original implementation of the sandbox for bug 341604.

So, I'll do that first and then get back to you, rather than taking up too much of your time.

How do I assign the bug to myself? Probably obvious, but I can't spot the way.

Thanks,
Bob
(In reply to Bob Owen from comment #5)

Hi Bob, first of all, thanks for taking a crack at this ! 

> I've realised this morning that I can get a lot of the information from
> reading through your original implementation of the sandbox for bug 341604.

yes, the main patch in bug 341604 is a good place to start.

> How do I assign the bug to myself? Probably obvious, but I can't spot the
> way.

you can click 'take' to the right of the Assigned To: field above and change the status to ASSIGNED.

> I'm guessing I'll need to add another flag to content/base/src/nsSandboxFlags.h
> Then modify ParseSandboxAttributeToFlags in content/base/src/nsContentUtils.cpp to 
> parse and set this flag.

yep, that's right - Bug 784402 may be another good reference too, this added 'allow-pointer-lock' to iframe sandbox. 

> I'm also guessing this is the easy part.

indeed :)

> What I don't really have any idea on yet, is where I need to check this flag and allow > the creation of the popup

right now the iframe sandbox checks that stop new browsing contexts from being created check the SANDBOXED_NAVIGATION flag. You will want to look at the checks in 
nsWindowWatcher::OpenWindowJSInternal and nsDocShell::InternalLoad and possibly a few other places. 

There are a few wrinkles with allow-popups - one of which is that windows created by a sandboxed document need to be sandboxed the same as the document which opened them, see http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#sandboxWindowOpen :

"If the current browsing context's active document's active sandboxing flag set has the sandboxed navigation browsing context flag set and chosen browsing context picked above, if any, is a new browsing context (whether top-level or auxiliary), then all the flags that are set in the current browsing context's active document's active sandboxing flag set when the new browsing context is created must be set in the new browsing context's popup sandboxing flag set, and the current browsing context must be set as the new browsing context's one permitted sandboxed navigator."

As this also says, only the browsing context that opened the new window is allowed to navigate it - this will require additional work in bug 785310 which I'm currently working on, but don't worry about that at the moment :) 

And of course, this will need tests ! The existing tests around blocking window opening from a sandboxed document are in http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_iframe_sandbox_general.html, specifically file_iframe_sandbox_c_if4.html.
Thanks Ian,

I'll look into these in the morning.

I don't seem to have the option to "take" the bug.

I've only just registered my account, perhaps I don't have the permissions yet.

Cheers,
Bob
(In reply to Bob Owen from comment #7)
> Thanks Ian,
> 
> I'll look into these in the morning.
> 
> I don't seem to have the option to "take" the bug.
> 
> I've only just registered my account, perhaps I don't have the permissions
> yet.

ah right - yes, that would be the case. I'll assign it to you for now.
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
(In reply to Ian Melven :imelven from comment #6)
> right now the iframe sandbox checks that stop new browsing contexts from
> being created check the SANDBOXED_NAVIGATION flag. You will want to look at
> the checks in 
> nsWindowWatcher::OpenWindowJSInternal and nsDocShell::InternalLoad and
> possibly a few other places. 

Haven't had much time today.
I've been replacing the capacitors in the power supply on my Topfield PVR (looked liked they were about to fail just before Christmas!) and I had to take my dog to the vet.

I'd got as far as those two classes yesterday and changed it to check the new flag instead.
It wasn't working and I couldn't figure out why.  I've just realised it is because yesterday I already had the standard Firefox running and when you run the local build it seems to use the already running code.
So, it is now passing the related tests on 
http://samples.msdn.microsoft.com/ietestcenter/#html5Sandbox 
I'm sure that doesn't cover everything.
 
> There are a few wrinkles with allow-popups - one of which is that windows
> created by a sandboxed document need to be sandboxed the same as the
> document which opened them

I'm going to try and get my head around the automated tests and the specification, to check all of this stuff first before I make any other changes.
I might not have too much time this week, but I intend to put some time in over the holidays (I don't expect responses if I do :) ).
(In reply to Bob Owen from comment #9)
>
> It wasn't working and I couldn't figure out why.  I've just realised it is
> because yesterday I already had the standard Firefox running and when you
> run the local build it seems to use the already running code.

You can get around this by passing '-no-remote' as an argument when you launch your local build, e.g. objdir-dbg/dist/bin/firefox.exe -no-remote

You will also want to use a different profile to the one in use with your release Firefox, you can pass -P to launch the profile manager or -p to use a different profile.
Here are the changes with a new SANDBOXED_AUXILIARY_NAVIGATION flag, which is set off by "allow-popups" and is used when checking if a new window can be opened.  There are three basic tests added to the general tests.

I am reading through the "Loading Web pages" W3C spec when I get spare time, so that I can then create more tests to make sure any new browsing contexts have their sandbox flags set correctly and behave as expected.

Cheers,
Bob
Attachment #696636 - Flags: feedback?(imelven)
Comment on attachment 696636 [details] [diff] [review]
WIP - initial allow-popups with tests, needs sandbox inheritance testing.

Review of attachment 696636 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks good to me! There's a few very small nits, some of which you may have inherited from the existing tests.

I think working on tests for new browsing contexts being correctly sandboxed is a great next step.

::: content/base/src/nsGkAtomList.h
@@ +75,5 @@
>  GK_ATOM(allownegativeassertions, "allownegativeassertions")
>  GK_ATOM(allowfullscreen, "allowfullscreen")
>  GK_ATOM(allowsameorigin, "allow-same-origin")
>  GK_ATOM(allowscripts, "allow-scripts")
> +GK_ATOM(allowpopups, "allow-popups")

nit: these look like they're alphabetized so this should be before allowsameorigin

::: content/html/content/test/file_iframe_sandbox_c_if9.html
@@ +17,5 @@
> +    // The window we try to open closes itself once it opens.
> +    sendMouseEvent({type:'click'}, 'target_blank');
> +
> +    window.open("file_iframe_sandbox_open_window_pass.html");
> +    

small nit: extra whitespace here

::: content/html/content/test/file_iframe_sandbox_open_window_pass.html
@@ +2,5 @@
> +<html>
> +<head>
> +  <meta charset="utf-8">
> +  <title>Test for Bug 766282</title>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>

nit: we don't need SimpleTest.js here

@@ +13,5 @@
> +</html>
> +
> +<script>
> +  function doStuff() {
> +    console.log("file_iframe_sandbox_window_open_pass.html");

nit: we should probably take this out to reduce console spam during test runs
Attachment #696636 - Flags: feedback?(imelven) → feedback+
(In reply to Ian Melven :imelven from comment #12)
> Comment on attachment 696636 [details] [diff] [review]
> WIP - initial allow-popups with tests, needs sandbox inheritance testing.
> 
> Review of attachment 696636 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch looks good to me! There's a few very small nits, some of which
> you may have inherited from the existing tests.

Thanks and my fault for copying code I didn't fully understand or check :).
 
> ::: content/base/src/nsGkAtomList.h
> @@ +75,5 @@
> >  GK_ATOM(allownegativeassertions, "allownegativeassertions")
> >  GK_ATOM(allowfullscreen, "allowfullscreen")
> >  GK_ATOM(allowsameorigin, "allow-same-origin")
> >  GK_ATOM(allowscripts, "allow-scripts")
> > +GK_ATOM(allowpopups, "allow-popups")
> 
> nit: these look like they're alphabetized so this should be before
> allowsameorigin

Hadn't spotted that, I will fix it.
Although I wasn't sure whether to add this as I don't think it is being used.
Not sure if the original patch used these either or if they were a hangover from a previous version. 

> 
> ::: content/html/content/test/file_iframe_sandbox_c_if9.html
> @@ +17,5 @@
> > +    // The window we try to open closes itself once it opens.
> > +    sendMouseEvent({type:'click'}, 'target_blank');
> > +
> > +    window.open("file_iframe_sandbox_open_window_pass.html");
> > +    
> 
> small nit: extra whitespace here

Thanks, will fix.

> ::: content/html/content/test/file_iframe_sandbox_open_window_pass.html
> @@ +2,5 @@
> > +<html>
> > +<head>
> > +  <meta charset="utf-8">
> > +  <title>Test for Bug 766282</title>
> > +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> 
> nit: we don't need SimpleTest.js here

Thanks, you're right that this was from a copy from the fail version of this file.  As I said my fault for not checking what I was copying.  I need to spend some time to understand the testing framework more fully, so I don't make this sort of mistake.

Should I remove this from the fail version as well?

> 
> @@ +13,5 @@
> > +</html>
> > +
> > +<script>
> > +  function doStuff() {
> > +    console.log("file_iframe_sandbox_window_open_pass.html");
> 
> nit: we should probably take this out to reduce console spam during test runs

OK, I'll take it out.  Again ... should I remove this from the fail version as well?

Thanks,
Bob
(In reply to Bob Owen from comment #13)
>
> Thanks and my fault for copying code I didn't fully understand or check :).

nah, it's my fault for having that extra stuff still in there, my apologies :)
  
> > ::: content/base/src/nsGkAtomList.h
> > @@ +75,5 @@
> > >  GK_ATOM(allownegativeassertions, "allownegativeassertions")
> > >  GK_ATOM(allowfullscreen, "allowfullscreen")
> > >  GK_ATOM(allowsameorigin, "allow-same-origin")
> > >  GK_ATOM(allowscripts, "allow-scripts")
> > > +GK_ATOM(allowpopups, "allow-popups")
> > 
> > nit: these look like they're alphabetized so this should be before
> > allowsameorigin
> 
> Hadn't spotted that, I will fix it.
> Although I wasn't sure whether to add this as I don't think it is being used.
> Not sure if the original patch used these either or if they were a hangover
> from a previous version. 

yeah, i originally started off using atoms but on advice during reviews, I switched to using strings in ParseSandboxAttribute instead - so feel free to rip these out if indeed the atoms aren't used anywhere now. 

> > ::: content/html/content/test/file_iframe_sandbox_open_window_pass.html
> > @@ +2,5 @@
> > > +<html>
> > > +<head>
> > > +  <meta charset="utf-8">
> > > +  <title>Test for Bug 766282</title>
> > > +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> > 
> > nit: we don't need SimpleTest.js here
> 
> Thanks, you're right that this was from a copy from the fail version of this
> file.  As I said my fault for not checking what I was copying.  I need to
> spend some time to understand the testing framework more fully, so I don't
> make this sort of mistake.

https://developer.mozilla.org/en-US/docs/Mochitest is a good place to start

> Should I remove this from the fail version as well?

that would be great, thank you.

> > 
> > @@ +13,5 @@
> > > +</html>
> > > +
> > > +<script>
> > > +  function doStuff() {
> > > +    console.log("file_iframe_sandbox_window_open_pass.html");
> > 
> > nit: we should probably take this out to reduce console spam during test runs
> 
> OK, I'll take it out.  Again ... should I remove this from the fail version
> as well?

that would be very good of you :)

thanks !
Had some more time, so addressed those issues.
I removed all but the sandbox atom as the others were not used.

Finished reading most of the spec (at least the relevant parts) and added a new test to check that a new browsing context is opened when a non-existing browsing context name is given in the target attribute.
Changed the pass file to accept the browsing context name through a trivial query string for checking.
Added similar test when allow-popups not specified for completeness.

I notice that a couple of the files that I have modified have DOS line delimiters instead of Unix ones ... should I dos2unix them?

Next I'll work on tests for the sandbox flags and one permitted navigator on the new browsing context.

Cheers,
Bob
Attachment #696636 - Attachment is obsolete: true
Attachment #699876 - Flags: feedback?(imelven)
Comment on attachment 699876 [details] [diff] [review]
WIP - corrected issues in Comment 12 feedback, added two tests

(In reply to Bob Owen from comment #15)
> Created attachment 699876 [details] [diff] [review]
> WIP - corrected issues in Comment 12 feedback, added two tests
> 
> Had some more time, so addressed those issues.
> I removed all but the sandbox atom as the others were not used.

sounds good.

> Finished reading most of the spec (at least the relevant parts) and added a
> new test to check that a new browsing context is opened when a non-existing
> browsing context name is given in the target attribute.

Good catch on adding this test, thanks !

> Changed the pass file to accept the browsing context name through a trivial
> query string for checking.

seems solid to me.

> Added similar test when allow-popups not specified for completeness.

Thank you very much for this ! I missed that case originally.

> I notice that a couple of the files that I have modified have DOS line
> delimiters instead of Unix ones ... should I dos2unix them?

per https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style, yes please :)

> Next I'll work on tests for the sandbox flags and one permitted navigator on
> the new browsing context.

the one permitted navigator will be a little tricky - my suggestion would be to try in this patch to address the case where the navigation is done via the 'target' attribute. 

The case that looks like :

win = window.open('foo.html')
win.location = 'bar.html'

will need to be addressed by the same mechanism I'm working on in bug 785310 (tl;dr : use a special sandbox aware wrapper for the location object that does extra checks). 

The other piece we need to take care of is "If the current browsing context's active document's active sandboxing flag set has the sandboxed navigation browsing context flag set and chosen browsing context picked above, if any, is a new browsing context (whether top-level or auxiliary), then all the flags that are set in the current browsing context's active document's active sandboxing flag set when the new browsing context is created must be set in the new browsing context's popup sandboxing flag set" which at this point, I think boils down to making sure the sandbox flags from the document opening the window are copied to the docshell of the popup (and will then be copied to the document when it's loaded).
Attachment #699876 - Flags: feedback?(imelven) → feedback+
Added tests to test_iframe_sandbox_inheritance.html, to check the sandbox flags on the new browsing contexts, which should be inherited from the document that opened them.
They check different flags are copied correctly for 4 different ways of creating the new browsing contexts.
Also checks that the flags are copied from the active document not the browsing context (docShell).

Made modifications to pass the tests.
Tests took a bit of time, but quite satisfying when they all pass with only a few lines a code.

Had to change nsDocShell::FindItemWithName to return an ERROR when foundItem not allowed due to sandboxing.
This is so that in nsDocShell::InternalLoad, I can tell the difference between no browsing context being found and one that is found but not allowed due to sandboxing.
If there is genuinely no browsing context for the target, I open a new one (assuming allow-popups is specified), otherwise I return the same error.
I chose NS_ERROR_DOM_INVALID_ACCESS_ERR, as this is what is suggested in the specification.  Not sure if this will cause any unforeseen issues.

Had a problem using postMessage from file_iframe_sandbox_a_if12.html to if11 when it was opened via showModalDialog, so had to add a timeout to make sure it closes.
The error was:
[18:28:03.573] Error: Permission denied for <moz-nullprincipal:{fcf29230-bf4e-41d5-a758-85b1febbc69b}> to call method ModalContentWindow.postMessage @ http://mochi.test:8888/tests/content/html/content/test/file_iframe_sandbox_a_if12.html:14

Also, ran dos2unix on:
  content/base/src/nsSandboxFlags.h
  content/html/content/test/test_iframe_sandbox_general.html
  content/html/content/test/test_iframe_sandbox_inheritance.html
Attachment #699876 - Attachment is obsolete: true
Attachment #702796 - Flags: feedback?(imelven)
<Comment 17>s/lines a code/lines of code/
(In reply to Bob Owen from comment #17)
> Created attachment 702796 [details] [diff] [review]
> WIP - one permitted sandboxed navigator functionality still required.
> 
> Added tests to test_iframe_sandbox_inheritance.html, to check the sandbox
> flags on the new browsing contexts, which should be inherited from the
> document that opened them.
> They check different flags are copied correctly for 4 different ways of
> creating the new browsing contexts.
> Also checks that the flags are copied from the active document not the
> browsing context (docShell).

very thorough, thank you !

> Made modifications to pass the tests.
> Tests took a bit of time, but quite satisfying when they all pass with only
> a few lines a code.

indeed, they can definitely be slow to write, but are so very worth it, IMO :) 

> Had to change nsDocShell::FindItemWithName to return an ERROR when foundItem
> not allowed due to sandboxing.
> This is so that in nsDocShell::InternalLoad, I can tell the difference
> between no browsing context being found and one that is found but not
> allowed due to sandboxing.
> If there is genuinely no browsing context for the target, I open a new one
> (assuming allow-popups is specified), otherwise I return the same error.
> I chose NS_ERROR_DOM_INVALID_ACCESS_ERR, as this is what is suggested in the
> specification.  Not sure if this will cause any unforeseen issues.

a try run will shake that out to some degree and maybe the reviewer will have some insight as well. 

> Had a problem using postMessage from file_iframe_sandbox_a_if12.html to if11
> when it was opened via showModalDialog, so had to add a timeout to make sure
> it closes.
> The error was:
> [18:28:03.573] Error: Permission denied for
> <moz-nullprincipal:{fcf29230-bf4e-41d5-a758-85b1febbc69b}> to call method
> ModalContentWindow.postMessage @
> http://mochi.test:8888/tests/content/html/content/test/
> file_iframe_sandbox_a_if12.html:14

interesting, I wonder why this is disallowed from a modal window, it may well be per spec. In general, try to avoid timeouts in the tests where possible to avoid intermittent failures (aka oranges). Not sure what else you can do in this situation - the test suite itself also has its own timeout so if the window doesn't close we're covered in that situation and the whole test file will fail. Not sure if we should rely on that to catch this test failing. The reviewer may well have some input here as well.

> Also, ran dos2unix on:
>   content/base/src/nsSandboxFlags.h
>   content/html/content/test/test_iframe_sandbox_general.html
>   content/html/content/test/test_iframe_sandbox_inheritance.html

Thanks very much for cleaning that up ! 

bug 785310 just went through another round of review from bholley and I have more work to do there - if you wanted to take a crack at the 'one permitted navigator' functionality based on the WIP patch in that bug, feel free :)
Comment on attachment 702796 [details] [diff] [review]
WIP - one permitted sandboxed navigator functionality still required.

Review of attachment 702796 [details] [diff] [review]:
-----------------------------------------------------------------

A lot of the whitespace issues are still probably mine from the original test files.

Thanks again for covering so many potential cases in the tests ! 

Overall, this looks really solid - I'll take another closer look at the exact test cases you added to see if I can think of any more. 

Olli, could you take a look at this (ignoring the whitespace issues I found :) ) please ?

::: content/html/content/test/file_iframe_sandbox_a_if10.html
@@ +19,5 @@
> +</script>
> +<body onLoad="doStuff()">
> +  I am navigated to from file_iframe_sandbox_a_if17.html.
> +  This was opened in an iframe with "allow-scripts allow-popups allow-same-origin".
> +  However allow-same-origin was removed from the iframe before navigating to me, 

nit: trailing whitespace

I really like that you made sure to test that the flags on the document, not on the docshell, are inherited,
as you mentioned.

@@ +20,5 @@
> +<body onLoad="doStuff()">
> +  I am navigated to from file_iframe_sandbox_a_if17.html.
> +  This was opened in an iframe with "allow-scripts allow-popups allow-same-origin".
> +  However allow-same-origin was removed from the iframe before navigating to me, 
> +  so I should only have "allow-scripts allow-popups" in force. 

nit: trailing whitespace

::: content/html/content/test/file_iframe_sandbox_a_if12.html
@@ +8,5 @@
> +</head>
> +<script type="application/javascript">
> +  function doTest() {
> +    sendMouseEvent({type:'click'}, 'anchor');
> +    

nit: whitespace

::: content/html/content/test/file_iframe_sandbox_a_if17.html
@@ +23,5 @@
> +  window.doSubOpens = doSubOpens;
> +</script>
> +
> +<body>
> +  I am sandboxed but with "allow-scripts allow-popups allow-same-origin". 

nit: trailing whitespace

@@ +24,5 @@
> +</script>
> +
> +<body>
> +  I am sandboxed but with "allow-scripts allow-popups allow-same-origin". 
> +  After my initial load, "allow-same-origin" is removed and then I open file_iframe_sandbox_a_if18.html 

nit: trailing whitespace

@@ +25,5 @@
> +
> +<body>
> +  I am sandboxed but with "allow-scripts allow-popups allow-same-origin". 
> +  After my initial load, "allow-same-origin" is removed and then I open file_iframe_sandbox_a_if18.html 
> +  in 4 different ways, which attemps to call a function in my parent. 

nit: trailing whitespace

::: docshell/base/nsDocShell.cpp
@@ +8534,4 @@
>          // don't try inheriting an owner from the target window if we came up
>          // with a null owner above.
>          aFlags = aFlags & ~INTERNAL_LOAD_FLAGS_INHERIT_OWNER;
>          

stray tab, but predates your work, feel free to clean up

@@ +8546,3 @@
>  
>          nsCOMPtr<nsIDocShell> targetDocShell = do_QueryInterface(targetItem);
>          

stray tab, but predates your work, feel free to clean up

::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp
@@ +609,5 @@
>  
>      callerContextGuard.Push(cx);
>    }
>  
> +  uint32_t activeDocsSandboxFlags = 0; 

nit: trailing whitespace

@@ +616,5 @@
>      // nsIWindowProvider for one.  In either case, we'll want to set the right
>      // name on it.
>      windowNeedsName = true;
>  
>      // If the parent trying to open a new window is sandboxed,

maybe add "without 'allow-popups'" to this line of comment

@@ +783,1 @@
>    

whitespace, but again not your fault.
Attachment #702796 - Flags: review?(bugs)
Attachment #702796 - Flags: feedback?(imelven)
Attachment #702796 - Flags: feedback+
cc'ing bz in case he's interested, apologies if not, Boris !
Verified that all the iframe sandbox tests passed locally and thought we might give Bob's patch a go on try to see where we're at:

https://tbpl.mozilla.org/?tree=Try&rev=84723ed193b4
(In reply to Ian Melven :imelven from comment #20)
> Comment on attachment 702796 [details] [diff] [review]
> WIP - one permitted sandboxed navigator functionality still required.
> 
> Review of attachment 702796 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A lot of the whitespace issues are still probably mine from the original
> test files.

You're too kind, these are definitely mine :).
I wasn't worrying about the trailing spaces in html, bit sloppy, I'll clean those up.

> 
> Thanks again for covering so many potential cases in the tests ! 

I tried to write most of them first, so I wasn't making assumptions about how the code works.

> 
> Overall, this looks really solid - I'll take another closer look at the
> exact test cases you added to see if I can think of any more. 

Thanks.

> ::: docshell/base/nsDocShell.cpp
> @@ +8534,4 @@
> >          // don't try inheriting an owner from the target window if we came up
> >          // with a null owner above.
> >          aFlags = aFlags & ~INTERNAL_LOAD_FLAGS_INHERIT_OWNER;
> >          
> 
> stray tab, but predates your work, feel free to clean up
> 
> @@ +8546,3 @@
> >  
> >          nsCOMPtr<nsIDocShell> targetDocShell = do_QueryInterface(targetItem);
> >          
> 
> stray tab, but predates your work, feel free to clean up

I noticed a few whitespace / tab issues in some of the existing code, but I thought that cleaning it all up would make the patch more difficult to follow.  Shall I do it anyway or would it be an idea to put in a different patch or perhaps raise a separate bug and sort it afterwards.

> 
> ::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp
> @@ +609,5 @@
> >  
> >      callerContextGuard.Push(cx);
> >    }
> >  
> > +  uint32_t activeDocsSandboxFlags = 0; 
> 
> nit: trailing whitespace
> 

Damn, don't know how I missed that one, thanks.

> @@ +616,5 @@
> >      // nsIWindowProvider for one.  In either case, we'll want to set the right
> >      // name on it.
> >      windowNeedsName = true;
> >  
> >      // If the parent trying to open a new window is sandboxed,
> 
> maybe add "without 'allow-popups'" to this line of comment

Yes, didn't spot that I'd made that comment incorrect with my change.

Thanks Ian, I'll try and sort all that out today or tomorrow and maybe even get some of the one permitted sandboxed navigator part done as well.
Comment on attachment 702796 [details] [diff] [review]
WIP - one permitted sandboxed navigator functionality still required.

So there is a new patch coming? I could review that.
Attachment #702796 - Flags: review?(bugs)
(In reply to Bob Owen from comment #23) 
> I noticed a few whitespace / tab issues in some of the existing code, but I
> thought that cleaning it all up would make the patch more difficult to
> follow.  Shall I do it anyway or would it be an idea to put in a different
> patch or perhaps raise a separate bug and sort it afterwards.
No need to fix use of tabs in existing code.
Someone was asking about this bug - yes, there is a new patch coming at some point.
Attached patch allow-popups tests (obsolete) — Splinter Review
Sorry it's taken so long to get this patch out.
I was initially delayed by finding and fixing bur 838692.
I've also just started a new contract so my time is now fairly limited.
Attachment for code changes to follow.
Attachment #728429 - Flags: feedback?(imelven)
Attached patch allow-popups code changes (obsolete) — Splinter Review
Implementation now includes one permitted sandboxed navigator functionality.
Also refactored sandbox flag checking for nsDocShell::FindItemWithName().

As per the previous implementation I am checking against |this| in nsDocShell::IsInSandbox(), but I wonder if this should be using |accessingItem|, which is |aOriginalRequestor| from FindItemWithName().

I suggest that the case for the "one permitted sandboxed navigator" and x.location = <url> are picked up in bug 785310.
As this is not stopped by sandboxing at the moment, I think this will work for the "one permitted sandboxed navigator" currently.
When bug 785310 puts the security in, it needs to make sure it doesn't block it for this navigator.
Attachment #702796 - Attachment is obsolete: true
Attachment #728440 - Flags: feedback?(imelven)
(In reply to Bob Owen from comment #28)

I forgot to mention that these changes have been done on top of the changes for bugs 849791 and 838692.

So, the patches from these bugs need to be applied first in this order:
bug 849791
bug 838692


Cheers,
Bob
Depends on: 859454
Blocks: framesandbox
No longer depends on: framesandbox
Attachment #728440 - Attachment is patch: true
Hi, I just installed FireFox version 21. I'm still getting the same problem Chris Foote described in COMMENT 1.
Attached patch allow-popups tests (obsolete) — Splinter Review
This patch has been done on top of the patch for bug 885140, as this will probably land before this one.
Attachment #728429 - Attachment is obsolete: true
Attachment #728429 - Flags: feedback?(imelven)
Attachment #769501 - Flags: feedback?(imelven)
Attached patch allow-popups code changes (obsolete) — Splinter Review
Patches merged with latest changes on mainline.

As I've explained in Comment 28, I think that we should be ready to start looking at landing this.
This assumes that bug 785310 allows for the "one permitted sandboxed navigator" in its implementation.

I'm still trying to check that the tests don't cause a problem on B2G.
Attachment #728440 - Attachment is obsolete: true
Attachment #728440 - Flags: feedback?(imelven)
Attachment #769503 - Flags: feedback?(imelven)
Here's a couple of limited try runs:

https://tbpl.mozilla.org/?tree=Try&rev=1c077ef7c767

https://tbpl.mozilla.org/?tree=Try&rev=18b036ec2e11

Had to do the second one because the tests don't seem to run for B2G debug.

content/html/content/test/test_iframe_sandbox_general.html now timing out on B2G.
This will be down to the limit on the number of open windows.
Other sandbox tests are already skipped for B2G for this reason.
I don't want to ignore all of general; when I get a second I'll split my new tests out into a popups file, so that at least I don't reduce the test coverage.

As I've said after similar issues in bug 838692, after I get this bug landed I intend to try and rework all the sandbox tests, so they can run on B2G.

Cheers,
Bob
Try run with allow-popups tests split out from the general ones and skipped for B2G.
I've checked the log and the content/html/content/test/test_iframe_sandbox_general.html tests are still running.

https://tbpl.mozilla.org/?tree=Try&rev=7e9335680f02

I will upload the new tests patch after work tonight and do a wider try run.
Attached patch allow-popups tests (obsolete) — Splinter Review
Split out popup tests from general ones, so they could be skipped for B2G.

New try run:

https://tbpl.mozilla.org/?tree=Try&rev=1adcc4a68762
Attachment #769501 - Attachment is obsolete: true
Attachment #769501 - Flags: feedback?(imelven)
Attachment #770377 - Flags: feedback?(imelven)
(In reply to Bob Owen from comment #36)

Looks like Firefox on Android doesn't like window.showModalDialog().

Should I remove the modal testing altogether for now or skip these tests on Android?
If I have time, then I'll see if it will be quick to split out the modal tests into a separate file, so i can just skip those for Android.

All the other failures, look like standard failures that worked after a retry.
It also looks like there was some sort of system error for the B2G Arm (VM) opt tests.
testing showModalDialog only on desktop sounds good.
I knew that would be the request as soon as I'd typed it :)


Quite right too ... I'll get onto it next time I get a chance.
Attached patch allow-popups code changes (obsolete) — Splinter Review
Merged with latest mainline changes.
Removed mistaken double call to FindItemWithName after last merge.
Fixed comment that no longer made sense with latest changes.
Attachment #769503 - Attachment is obsolete: true
Attachment #769503 - Flags: feedback?(imelven)
Attachment #771825 - Flags: feedback?(imelven)
Attached patch allow-popups tests (obsolete) — Splinter Review
Split out failing showModalDialog tests from test_iframe_sandbox_inheritance.html and test_iframe_sandbox_popups.html into new test_iframe_sandbox_modal.html file.
These are now skipped for Android and B2G.

New try run:
https://tbpl.mozilla.org/?tree=Try&rev=3b4d5be76e45

Everything looks OK after a few retries.

Cheers,
Bob
Attachment #770377 - Attachment is obsolete: true
Attachment #770377 - Flags: feedback?(imelven)
Attachment #771878 - Flags: feedback?(imelven)
I need to merge the tests for this with the changes from bug 886262.

It'll be at least a week before I can do this, but I'll then put this forward for review.
Attached patch allow-popups tests (obsolete) — Splinter Review
Merged patch with test changes from bug 886262.
New fix patch to follow.

Olli, I'm hoping you are OK to review these for me?
Attachment #771878 - Attachment is obsolete: true
Attachment #771878 - Flags: feedback?(ian.melven)
Attachment #793019 - Flags: review?(bugs)
Attached patch allow-popups code changes (obsolete) — Splinter Review
Code changes merged with latest changes from mozilla-central.
Attachment #771825 - Attachment is obsolete: true
Attachment #771825 - Flags: feedback?(ian.melven)
Attachment #793020 - Flags: review?(bugs)
Fairly wide try run for these latest patches:
https://tbpl.mozilla.org/?tree=Try&rev=817a094b6125
Comment on attachment 793019 [details] [diff] [review]
allow-popups tests

Hmm, why is this patch so huge?
Something odd with test_iframe_sandbox_inheritance.html at least?
Are you changing newlines or what? Really hard to review this kind of patch.
Attachment #793019 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #46)
> Comment on attachment 793019 [details] [diff] [review]
> allow-popups tests
> 
> Hmm, why is this patch so huge?
> Something odd with test_iframe_sandbox_inheritance.html at least?
> Are you changing newlines or what? Really hard to review this kind of patch.

Yes ... sorry, the old version had DOS line endings.
I'll convert this and I think some other files back and upload a new patch when I get back from work tonight, or possibly in the morning.

Should I raise a separate bug so that the line endings can be cleaned up afterwards?
Just a separate patch for the line ending fix here is enough.
Attached patch allow-popups tests (obsolete) — Splinter Review
Split the fixing of line endings out into separate patch.
Attachment #793019 - Attachment is obsolete: true
Attachment #794242 - Flags: review?(bugs)
Should be applied after bug766282tests.patch
Attachment #794243 - Flags: review?(bugs)
Attached patch allow-popups code changes (obsolete) — Splinter Review
Merged with latest changes from mozilla-central including patch by bz for bug 907892.

New try run: https://tbpl.mozilla.org/?tree=Try&rev=0283609ed631

This was done on top of patch for bug 907892, as this had not been merged at that point, and without the line endings patch, but neither of these should make any difference.
Attachment #793020 - Attachment is obsolete: true
Attachment #793020 - Flags: review?(bugs)
Attachment #794247 - Flags: review?(bugs)
Thanks, I'll review this stuff tomorrow.
Thanks Olli.
I'm going to be away for just over a week.
I'll try and keep track of any comments, but I won't be able to make any changes until I get back.
Attachment #794243 - Flags: review?(bugs) → review+
Comment on attachment 794247 [details] [diff] [review]
allow-popups code changes


>+
>+/**
>+ * This flag prevents content from creating new auxiliary browsing contexts,
>+ * e.g. using the target attribute, the window.open() method, or the
>+ * showModalDialog() method.
>+ */
>+const unsigned long SANDBOXED_AUXILIARY_NAVIGATION = 0x200;
SANDBOXED_AUXILIARY_NAVIGATION is somewhat odd name, but I see it is coming from the
spec.

>+nsDocShell::IsInSandbox(nsIDocShellTreeItem* targetItem,
>+                        nsIDocShellTreeItem* accessingItem)
>+{
>+    uint32_t sandboxFlags = 0;
>+
>+    nsCOMPtr<nsIDocument> doc = do_GetInterface(accessingItem);
>+    if (doc) {
>+        sandboxFlags = doc->GetSandboxFlags();
>+    }
>+
>+    // If no flags, our sandbox is infinite.
>+    if (!sandboxFlags) {
>+        return true;
>+    }
I don't understand this. The method name is IsInSandbox and we return true if there are no sandbox flags at all?


>     uint32_t                   mSandboxFlags;
>+    nsIDocShell*               mOnePermittedSandboxedNavigator;
This is oddly named, but again name comes from the spec...
Also, this will lead to security bugs. Raw pointer to another object and we're not making sure to set
it null when the object dies.
Attachment #794247 - Flags: review?(bugs) → review-
Comment on attachment 794242 [details] [diff] [review]
allow-popups tests

I'll review this once I've given r+ to the patch.
Attachment #794242 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #54)
> Comment on attachment 794247 [details] [diff] [review]
> allow-popups code change
> 

Thanks Olli.

> >+nsDocShell::IsInSandbox(nsIDocShellTreeItem* targetItem,
> >+                        nsIDocShellTreeItem* accessingItem)
> >+{
> >+    uint32_t sandboxFlags = 0;
> >+
> >+    nsCOMPtr<nsIDocument> doc = do_GetInterface(accessingItem);
> >+    if (doc) {
> >+        sandboxFlags = doc->GetSandboxFlags();
> >+    }
> >+
> >+    // If no flags, our sandbox is infinite.
> >+    if (!sandboxFlags) {
> >+        return true;
> >+    }
> I don't understand this. The method name is IsInSandbox and we return true
> if there are no sandbox flags at all?
> 

It made sense to me to handle all the logic to do with checking sandboxing in this function.
So as having no sandbox is effectively the same as having one of infinite size, we return true here.

However, this is clearly a poor choice of name as it has already caused confusion.
I think it would be better if I changed the name to IsSandboxedFrom and flip all of the returns.
I`ll change the order of the parameters as well.

I wrote all of this about five months ago and this reminds me of another question.
I have reproduced the original functionality in this function, where the flags are taken from |accessingItem| (originally |aOriginalRequestor| in FindItemWithName, I think)  but |this| is used to compare with the objects.
Is this correct? 
I'm not sure if they are always the same. 
> 
> >     uint32_t                   mSandboxFlags;
> >+    nsIDocShell*               mOnePermittedSandboxedNavigator;
> This is oddly named, but again name comes from the spec...
> Also, this will lead to security bugs. Raw pointer to another object and
> we're not making sure to set
> it null when the object dies.

Ah ... thanks, I'll fix this when I get back from holiday as well.
Attached patch allow-popups tests (obsolete) — Splinter Review
Merged with latest changes from mozilla-central.
Attachment #794242 - Attachment is obsolete: true
Attached patch allow-popups code changes (obsolete) — Splinter Review
As per Comment 56:
Changed new nsDocShell function IsInSandbox to IsSandboxedFrom, updating return values, comments and caller to match.
Also moved the check to see if target is us to the top of the function.  This is the quickest check and probably the most common scenario, so I think it makes sense to do it before we even check the sandbox flags.

Set new raw pointer mOnePermittedSandboxedNavigator to nullptr in Destroy().


Reminder of question from Comment 56 in case it's been missed:
"I have reproduced the original functionality in this function, where the flags are taken from |accessingItem| (originally |aOriginalRequestor| in FindItemWithName, I think)  but |this| is used to compare with the objects.
Is this correct? 
I'm not sure if they are always the same."


New try run:
https://tbpl.mozilla.org/?tree=Try&rev=4e967b141e80
Attachment #794247 - Attachment is obsolete: true
Attachment #798600 - Flags: review?(bugs)
Hmm, perhaps make that SameCOMIdentity() and not 'this' comparison.
Comment on attachment 798600 [details] [diff] [review]
allow-popups code changes


>+nsDocShell::IsSandboxedFrom(nsIDocShellTreeItem* accessingItem,
>+                            nsIDocShellTreeItem* targetItem)
aAccessingItem, aTargetItem

>+{
>+    // We cannot be sandboxed from ourselves.
>+    if (targetItem == this) {
SameCOMIdentity, to be safe

>+    // Are we the "one permitted sandboxed navigator", i.e. did we open it?
>+    nsCOMPtr<nsIDocShell> targetDocShell = do_QueryInterface(targetItem);
>+    nsCOMPtr<nsIDocShell> permittedNavigator;
>+    targetDocShell->GetOnePermittedSandboxedNavigator(getter_AddRefs(
>+                                                        permittedNavigator));
odd indentation.
perhaps 
targetDocShell->
    GetOnePermittedSandboxedNavigator(getter_AddRefs(permittedNavigator));

>+    if (this == permittedNavigator) {
>+        return false;
>+    }
Why comparison to 'this'?
Shouldn't you compare to accessing item to the one permitted navigator?



>+    // If targetItem is our top, then we are sandboxed from it if toplevel flag
>+    // is on (i.e. when allow-top-navigation is not specified).
>+    nsCOMPtr<nsIDocShellTreeItem> rootTreeItem;
>+    GetSameTypeRootTreeItem(getter_AddRefs(rootTreeItem));
>+    if (targetItem == rootTreeItem) {
>+        return (sandboxFlags & SANDBOXED_TOPLEVEL_NAVIGATION);
>+    }
Should you get the root from the accessing item?

>+    // If we are an ancestor of the item, then we are not sandboxed from it.
>+    nsCOMPtr<nsIDocShellTreeItem> parentAsItem;
>+    targetItem->GetSameTypeParent(getter_AddRefs(parentAsItem));
>+    while (parentAsItem) {
>+        if (parentAsItem == this) {
Why this and not accessing item?

I think the current code has a bug too to compare to 'this'

>+    nsIDocShell*               mOnePermittedSandboxedNavigator;
So this is still totally unsafe. Nothing ensures mOnePermittedSandboxedNavigator stays alive, and if
it doesn't, it starts to point to a deleted object.
I'd prefer using nsWeakPtr here.

>+    // Check whether accessing item is sandboxed from the target item.
>+    bool IsSandboxedFrom(nsIDocShellTreeItem* accessingItem,
>+                         nsIDocShellTreeItem* targetItem);
Parameter names should be in form aFoo
>-  SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow));
>+  if (SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow))
>+      == NS_ERROR_DOM_INVALID_ACCESS_ERR) {
== should be in the previous line
Attachment #798600 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #60)
> Comment on attachment 798600 [details] [diff] [review]
> allow-popups code changes

Thanks for the review Olli.
Sorry about the delay, I've had my last assignment for my current Maths MSc module to get finished, so it's absorbed all of my free time.
I think I've made most of the changes necessary (comments below), so I'll get the patch uploaded as soon as I can.
 
> 
> >+nsDocShell::IsSandboxedFrom(nsIDocShellTreeItem* accessingItem,
> >+                            nsIDocShellTreeItem* targetItem)
> aAccessingItem, aTargetItem

Missed that in the standards document, at least it was easy to fix :)

> >+{
> >+    // We cannot be sandboxed from ourselves.
> >+    if (targetItem == this) {
> SameCOMIdentity, to be safe

I hadn't seen this function before, I've used it for all the comparisons now.
 
> >+    // Are we the "one permitted sandboxed navigator", i.e. did we open it?
> >+    nsCOMPtr<nsIDocShell> targetDocShell = do_QueryInterface(targetItem);
> >+    nsCOMPtr<nsIDocShell> permittedNavigator;
> >+    targetDocShell->GetOnePermittedSandboxedNavigator(getter_AddRefs(
> >+                                                        permittedNavigator));
> odd indentation.
> perhaps 
> targetDocShell->
>     GetOnePermittedSandboxedNavigator(getter_AddRefs(permittedNavigator));

Yes, that looks better, thanks.

> >+    if (this == permittedNavigator) {
> >+        return false;
> >+    }
> Why comparison to 'this'?
> Shouldn't you compare to accessing item to the one permitted navigator?
>
> 
> >+    // If targetItem is our top, then we are sandboxed from it if toplevel flag
> >+    // is on (i.e. when allow-top-navigation is not specified).
> >+    nsCOMPtr<nsIDocShellTreeItem> rootTreeItem;
> >+    GetSameTypeRootTreeItem(getter_AddRefs(rootTreeItem));
> >+    if (targetItem == rootTreeItem) {
> >+        return (sandboxFlags & SANDBOXED_TOPLEVEL_NAVIGATION);
> >+    }
> Should you get the root from the accessing item?
> 
> >+    // If we are an ancestor of the item, then we are not sandboxed from it.
> >+    nsCOMPtr<nsIDocShellTreeItem> parentAsItem;
> >+    targetItem->GetSameTypeParent(getter_AddRefs(parentAsItem));
> >+    while (parentAsItem) {
> >+        if (parentAsItem == this) {
> Why this and not accessing item?
> 
> I think the current code has a bug too to compare to 'this'

I noticed this and first mentioned it back in Comment 28, I thought there must be some good reason, so I reproduced the existing functionality and added my code in a similar vein.
I think it should be |aAccessingItem|, especially since looking at CanAccessItem(), which has a similar function.
I've changed it for all the uses of |this| and made the function static.
I've switched the parameters back round again to match CanAccessItem().

> 
> >+    nsIDocShell*               mOnePermittedSandboxedNavigator;
> So this is still totally unsafe. Nothing ensures
> mOnePermittedSandboxedNavigator stays alive, and if
> it doesn't, it starts to point to a deleted object.
> I'd prefer using nsWeakPtr here.

I did wonder about using nsWeakPtr, but wasn't sure it was necessary given the usage of |mOnePermittedSandboxedNavigator|.
I've changed this now ... I think I'm using it correctly.
I also added some checking into SetOnePermittedSandboxedNavigator() to raise an NS_ERROR if it gets set twice ... and it turns out it was already, so I've been able to remove the code for that from nsDocShell :)

> >-  SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow));
> >+  if (SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow))
> >+      == NS_ERROR_DOM_INVALID_ACCESS_ERR) {
> == should be in the previous line

Fixed, thanks.
Attached patch allow-popups code changes (obsolete) — Splinter Review
nsDocShell::IsSandboxedFrom():
* Used |aAccessingItem| instead of |this| and made function static.
* Used SameCOMIdentity() for comparisons.
* Corrected parameters to match naming convention and switched them back to match CanAccessItem().

mOnePermittedSandboxedNavigator:
* Changed to nsWeakPtr and changed Getter and Setter to use it.
* Also added NS_ERROR in Setter, if an attempt is made to set it twice.
* Removed setting in nsDocShell as it is already set in nsWindowWatcher.

Also fixed a couple of formatting issues.


New try run: https://tbpl.mozilla.org/?tree=Try&rev=d36acb45b57b
Attachment #798600 - Attachment is obsolete: true
Attachment #803908 - Flags: review?(bugs)
Comment on attachment 803908 [details] [diff] [review]
allow-popups code changes

So after this patch we don't use SANDBOXED_NAVIGATION for anything else than just having the sandbox non-zero.
Could you perhaps add a comment about it.

>+    mOnePermittedSandboxedNavigator(nullptr),
this isn't needed
>   /**
>+   * When a new browsing context is opened by a sandboxed document, it needs to
>+   * keep track of the browsing context that opened it, so that it can be
>+   * navigated by it.  This is the "one permitted sandboxed navigator".
>+   */
>+  attribute nsIDocShell onePermittedSandboxedNavigator;
Actually... is this ever different from window.opener? outerwindow which docshell has pointer to, keeps mOpener alive.
If mOpener is already the right thing, we should just use that.
Sorry that I didn't think about this before
(I guess I'll need to re-read the spec., but please do you too ;) )
Please re-ask review after answering to this or uploading a new patch.
Attachment #803908 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #63)
> Comment on attachment 803908 [details] [diff] [review]
> allow-popups code changes
> 
> So after this patch we don't use SANDBOXED_NAVIGATION for anything else than
> just having the sandbox non-zero.
> Could you perhaps add a comment about it.

Yes, it is used only when the flags are originally set and then implicitly when checking if the flags are non-zero, as this flag is always on when sandboxed.
I'll change the comment to make this clear.
 
> >+    mOnePermittedSandboxedNavigator(nullptr),
> this isn't needed

I did wonder ... I'll remove it, thanks.

> >   /**
> >+   * When a new browsing context is opened by a sandboxed document, it needs to
> >+   * keep track of the browsing context that opened it, so that it can be
> >+   * navigated by it.  This is the "one permitted sandboxed navigator".
> >+   */
> >+  attribute nsIDocShell onePermittedSandboxedNavigator;
> Actually... is this ever different from window.opener? outerwindow which
> docshell has pointer to, keeps mOpener alive.
> If mOpener is already the right thing, we should just use that.
> Sorry that I didn't think about this before
> (I guess I'll need to re-read the spec., but please do you too ;) )
> Please re-ask review after answering to this or uploading a new patch.

I did look at using mOpener, but it can be set to null (called disowning in the spec), so it doesn't quite work for this change.
Actually, if another browsing context uses window.open() to navigate a browsing context it didn't open it gets set as the opener, which is also a problem.  I think this is a bug and did mention it in bug 838692 comment 15:

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

Anyway, I'll try and get the two changes above done tomorrow and get a new patch uploaded.
nsSandboxFlags.h:
* Changed comment on SANDBOXED_NAVIGATION to explain its implicit use in the code.

nsDocShell.cpp:
* Removed unnecessary initialisation of mOnePermittedSandboxedNavigator to nullptr.
* While thinking about new comment above, I realised that I could reduce the checking done in IsSandboxedFrom(), so this has been changed around a bit as well ... sorry.

New try run: https://tbpl.mozilla.org/?tree=Try&rev=ecb13dded996
Attachment #803908 - Attachment is obsolete: true
Attachment #805554 - Flags: review?(bugs)
Comment on attachment 805554 [details] [diff] [review]
allow-popups code changes


>+  // Set up sandboxing attributes if the window is new.
>+  // The flags can only be non-zero for new windows.
>+  if (activeDocsSandboxFlags != 0) {
>+    newDocShell->SetSandboxFlags(activeDocsSandboxFlags);
I couldn't actually find in the spec what requires this behavior. It makes sense, but
I'd like the know where it is spec'ed
(In reply to Olli Pettay [:smaug] from comment #66)
> Comment on attachment 805554 [details] [diff] [review]
> allow-popups code changes
> 
> 
> >+  // Set up sandboxing attributes if the window is new.
> >+  // The flags can only be non-zero for new windows.
> >+  if (activeDocsSandboxFlags != 0) {
> >+    newDocShell->SetSandboxFlags(activeDocsSandboxFlags);
> I couldn't actually find in the spec what requires this behavior. It makes
> sense, but
> I'd like the know where it is spec'ed

Hi Olli, it's in the very last paragraph of:

http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#the-rules-for-choosing-a-browsing-context-given-a-browsing-context-name
Comment on attachment 805554 [details] [diff] [review]
allow-popups code changes

># HG changeset patch
># User Bob Owen <bobowencode@gmail.com>
># Date 1363891498 0
># Node ID d6457b15d1b5cc5449fdc0b7bb9d5abd862e0328
># Parent  80a979d90f0c0a655c9423625945af9c2fa679fe
>Fix for bug 766282 - implement allow-popups directive for iframe sandbox
>
>diff --git a/content/base/src/nsContentUtils.cpp b/content/base/src/nsContentUtils.cpp
>--- a/content/base/src/nsContentUtils.cpp
>+++ b/content/base/src/nsContentUtils.cpp
>@@ -917,16 +917,17 @@ nsContentUtils::GetParserService()
>  * @return              the set of flags
>  */
> uint32_t
> nsContentUtils::ParseSandboxAttributeToFlags(const nsAString& aSandboxAttrValue)
> {
>   // If there's a sandbox attribute at all (and there is if this is being
>   // called), start off by setting all the restriction flags.
>   uint32_t out = SANDBOXED_NAVIGATION |
>+                 SANDBOXED_AUXILIARY_NAVIGATION |
>                  SANDBOXED_TOPLEVEL_NAVIGATION |
>                  SANDBOXED_PLUGINS |
>                  SANDBOXED_ORIGIN |
>                  SANDBOXED_FORMS |
>                  SANDBOXED_SCRIPTS |
>                  SANDBOXED_AUTOMATIC_FEATURES |
>                  SANDBOXED_POINTER_LOCK |
>                  SANDBOXED_DOMAIN;
>@@ -947,16 +948,18 @@ nsContentUtils::ParseSandboxAttributeToF
>         // allow-scripts removes both SANDBOXED_SCRIPTS and
>         // SANDBOXED_AUTOMATIC_FEATURES.
>         out &= ~SANDBOXED_SCRIPTS;
>         out &= ~SANDBOXED_AUTOMATIC_FEATURES;
>       } else if (token.LowerCaseEqualsLiteral("allow-top-navigation")) {
>         out &= ~SANDBOXED_TOPLEVEL_NAVIGATION;
>       } else if (token.LowerCaseEqualsLiteral("allow-pointer-lock")) {
>         out &= ~SANDBOXED_POINTER_LOCK;
>+      } else if (token.LowerCaseEqualsLiteral("allow-popups")) {
>+        out &= ~SANDBOXED_AUXILIARY_NAVIGATION;
>       }
>     }
>   }
> 
>   return out;
> }
> 
> #ifdef IBMBIDI
>diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
>--- a/content/base/src/nsGkAtomList.h
>+++ b/content/base/src/nsGkAtomList.h
>@@ -67,22 +67,18 @@ GK_ATOM(actuate, "actuate")
> GK_ATOM(address, "address")
> GK_ATOM(after, "after")
> GK_ATOM(after_end, "after_end")
> GK_ATOM(after_start, "after_start")
> GK_ATOM(align, "align")
> GK_ATOM(alink, "alink")
> GK_ATOM(all, "all")
> GK_ATOM(allowevents, "allowevents")
>-GK_ATOM(allowforms, "allow-forms")
> GK_ATOM(allownegativeassertions, "allownegativeassertions")
> GK_ATOM(allowfullscreen, "allowfullscreen")
>-GK_ATOM(allowsameorigin, "allow-same-origin")
>-GK_ATOM(allowscripts, "allow-scripts")
>-GK_ATOM(allowtopnavigation, "allow-top-navigation")
> GK_ATOM(allowuntrusted, "allowuntrusted")
> GK_ATOM(alt, "alt")
> GK_ATOM(alternate, "alternate")
> GK_ATOM(always, "always")
> GK_ATOM(ancestor, "ancestor")
> GK_ATOM(ancestorOrSelf, "ancestor-or-self")
> GK_ATOM(_and, "and")
> GK_ATOM(any, "any")
>diff --git a/content/base/src/nsSandboxFlags.h b/content/base/src/nsSandboxFlags.h
>--- a/content/base/src/nsSandboxFlags.h
>+++ b/content/base/src/nsSandboxFlags.h
>@@ -8,18 +8,21 @@
>  * HTML5 spec.
>  */
> 
> #ifndef nsSandboxFlags_h___
> #define nsSandboxFlags_h___
> 
> /**
>  * This flag prevents content from navigating browsing contexts other than
>- * the sandboxed browsing context itself (or browsing contexts further
>- * nested inside it), and the top-level browsing context.
>+ * itself, browsing contexts nested inside it, the top-level browsing context
>+ * and browsing contexts that it has opened.
>+ * As it is always on for sandboxed browsing contexts, it is used implicitly
>+ * within the code by checking that the overall flags are non-zero.
>+ * It is only uesd directly when the sandbox flags are initially set up.
>  */
> const unsigned long SANDBOXED_NAVIGATION  = 0x1;
> 
> /**
>  * This flag prevents content from navigating their top-level browsing
>  * context.
>  */
> const unsigned long SANDBOXED_TOPLEVEL_NAVIGATION = 0x2;
>@@ -60,9 +63,16 @@ const unsigned long SANDBOXED_AUTOMATIC_
>  * This flag blocks the document from acquiring pointerlock.
>  */
> const unsigned long SANDBOXED_POINTER_LOCK = 0x80;
> 
> /**
>  * This flag blocks the document from changing document.domain.
>  */
> const unsigned long SANDBOXED_DOMAIN = 0x100;
>+
>+/**
>+ * This flag prevents content from creating new auxiliary browsing contexts,
>+ * e.g. using the target attribute, the window.open() method, or the
>+ * showModalDialog() method.
>+ */
>+const unsigned long SANDBOXED_AUXILIARY_NAVIGATION = 0x200;
> #endif
>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp
>--- a/docshell/base/nsDocShell.cpp
>+++ b/docshell/base/nsDocShell.cpp
>@@ -3185,72 +3185,21 @@ nsDocShell::FindItemWithName(const PRUni
> 
>         if (foundItem && !CanAccessItem(foundItem, aOriginalRequestor)) {
>             foundItem = nullptr;
>         }
> 
>         // DoFindItemWithName only returns active items and we don't check if
>         // the item is active for the special cases.
>         if (foundItem) {
>-
>-            // If our document is sandboxed, we need to do some extra checks.
>-            uint32_t sandboxFlags = 0;
>-
>-            nsCOMPtr<nsIDocument> doc = do_GetInterface(aOriginalRequestor);
>-
>-            if (doc) {
>-                sandboxFlags = doc->GetSandboxFlags();
>+            if (IsSandboxedFrom(foundItem, aOriginalRequestor)) {
>+                return NS_ERROR_DOM_INVALID_ACCESS_ERR;
>+            } else {
>+                foundItem.swap(*_retval);
>             }
>-
>-            if (sandboxFlags) {
>-                nsCOMPtr<nsIDocShellTreeItem> root;
>-                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) {
>-                    // Are we an ancestor of the foundItem ?
>-                    bool isAncestor = false;
>-
>-                    nsCOMPtr<nsIDocShellTreeItem> parentAsItem;
>-                    foundItem->GetSameTypeParent(getter_AddRefs(parentAsItem));
>-                    while (parentAsItem) {
>-                        if (parentAsItem == selfAsItem) {
>-                            isAncestor = true;
>-                            break;
>-                        }
>-                        nsCOMPtr<nsIDocShellTreeItem> tmp = parentAsItem;
>-                        tmp->GetSameTypeParent(getter_AddRefs(parentAsItem));
>-                    }
>-
>-                    if (!isAncestor) {
>-                        // No, we are not an ancestor and our document is
>-                        // sandboxed, we can't allow this.
>-                        foundItem = nullptr;
>-                    }
>-                } else {
>-                    // 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));
>-                    }
>-                }
>-            }
>-
>-            foundItem.swap(*_retval);
>         }
>         return NS_OK;
>     }
> }
> 
> nsresult
> nsDocShell::DoFindItemWithName(const PRUnichar* aName,
>                                nsISupports* aRequestor,
>@@ -3313,16 +3262,80 @@ nsDocShell::DoFindItemWithName(const PRU
>     if (mTreeOwner && mTreeOwner != reqAsTreeOwner) {
>         return mTreeOwner->
>             FindItemWithName(aName, this, aOriginalRequestor, _retval);
>     }
> 
>     return NS_OK;
> }
> 
>+/* static */
>+bool
>+nsDocShell::IsSandboxedFrom(nsIDocShellTreeItem* aTargetItem,
>+                            nsIDocShellTreeItem* aAccessingItem)
>+{
>+    // aAccessingItem cannot be sandboxed from itself.
>+    if (SameCOMIdentity(aTargetItem, aAccessingItem)) {
>+        return false;
>+    }
>+
>+    uint32_t sandboxFlags = 0;
>+
>+    nsCOMPtr<nsIDocument> doc = do_GetInterface(aAccessingItem);
>+    if (doc) {
>+        sandboxFlags = doc->GetSandboxFlags();
>+    }
>+
>+    // If no flags, aAccessingItem is not sandboxed at all.
>+    if (!sandboxFlags) {
>+        return false;
>+    }
>+
>+    // If aTargetItem has an ancestor, it is not top level.
>+    nsCOMPtr<nsIDocShellTreeItem> ancestorOfTarget;
>+    aTargetItem->GetSameTypeParent(getter_AddRefs(ancestorOfTarget));
>+    if (ancestorOfTarget) {
>+        do {
>+            // aAccessingItem is not sandboxed if it is an ancestor of target.
>+            if (SameCOMIdentity(aAccessingItem, ancestorOfTarget)) {
>+                return false;
>+            }
>+            nsCOMPtr<nsIDocShellTreeItem> tempTreeItem;
>+            ancestorOfTarget->GetSameTypeParent(getter_AddRefs(tempTreeItem));
>+            tempTreeItem.swap(ancestorOfTarget);
>+        } while (ancestorOfTarget);
>+
>+        // Otherwise, aAccessingItem is sandboxed from aTargetItem.
>+        return true;
>+    }
>+
>+    // aTargetItem is top level, is aAccessingItem the "one permitted sandboxed
>+    // navigator", i.e. did aAccessingItem open aTargetItem?
>+    nsCOMPtr<nsIDocShell> targetDocShell = do_QueryInterface(aTargetItem);
>+    nsCOMPtr<nsIDocShell> permittedNavigator;
>+    targetDocShell->
>+        GetOnePermittedSandboxedNavigator(getter_AddRefs(permittedNavigator));
>+    if (SameCOMIdentity(aAccessingItem, permittedNavigator)) {
>+        return false;
>+    }
>+
>+    // If SANDBOXED_TOPLEVEL_NAVIGATION flag is not on, aAccessingItem is
>+    // not sandboxed from its top.
>+    if (!(sandboxFlags & SANDBOXED_TOPLEVEL_NAVIGATION)) {
>+        nsCOMPtr<nsIDocShellTreeItem> rootTreeItem;
>+        aAccessingItem->GetSameTypeRootTreeItem(getter_AddRefs(rootTreeItem));
>+        if (SameCOMIdentity(aTargetItem, rootTreeItem)) {
>+            return false;
>+        }
>+    }
>+
>+    // Otherwise, aAccessingItem is sandboxed from aTargetItem.
>+    return true;
>+}
>+
> NS_IMETHODIMP
> nsDocShell::GetTreeOwner(nsIDocShellTreeOwner ** aTreeOwner)
> {
>     NS_ENSURE_ARG_POINTER(aTreeOwner);
> 
>     *aTreeOwner = mTreeOwner;
>     NS_IF_ADDREF(*aTreeOwner);
>     return NS_OK;
>@@ -5022,16 +5035,18 @@ nsDocShell::Destroy()
>         if (shPrivate) {
>             shPrivate->EvictAllContentViewers();
>         }
>         mSessionHistory = nullptr;
>     }
> 
>     SetTreeOwner(nullptr);
> 
>+    mOnePermittedSandboxedNavigator = nullptr;
>+
>     // required to break ref cycle
>     mSecurityUI = nullptr;
> 
>     // Cancel any timers that were set for this docshell; this is needed
>     // to break the cycle between us and the timers.
>     CancelRefreshURITimers();
> 
>     if (mInPrivateBrowsing) {
>@@ -5371,16 +5386,41 @@ nsDocShell::SetSandboxFlags(uint32_t aSa
> NS_IMETHODIMP
> nsDocShell::GetSandboxFlags(uint32_t  *aSandboxFlags)
> {
>     *aSandboxFlags = mSandboxFlags;
>     return NS_OK;
> }
> 
> NS_IMETHODIMP
>+nsDocShell::SetOnePermittedSandboxedNavigator(nsIDocShell* aSandboxedNavigator)
>+{
>+    if (mOnePermittedSandboxedNavigator) {
>+        NS_ERROR("One Permitted Sandboxed Navigator should only be set once.");
>+        return NS_OK;
>+    }
>+
>+    mOnePermittedSandboxedNavigator = do_GetWeakReference(aSandboxedNavigator);
>+    NS_ASSERTION(mOnePermittedSandboxedNavigator,
>+             "One Permitted Sandboxed Navigator must support weak references.");
>+
>+    return NS_OK;
>+}
>+
>+NS_IMETHODIMP
>+nsDocShell::GetOnePermittedSandboxedNavigator(nsIDocShell** aSandboxedNavigator)
>+{
>+    NS_ENSURE_ARG_POINTER(aSandboxedNavigator);
>+    nsCOMPtr<nsIDocShell> permittedNavigator =
>+        do_QueryReferent(mOnePermittedSandboxedNavigator);
>+    NS_IF_ADDREF(*aSandboxedNavigator = permittedNavigator);
>+    return NS_OK;
>+}
>+
>+NS_IMETHODIMP
> nsDocShell::SetDefaultLoadFlags(uint32_t aDefaultLoadFlags)
> {
>     mDefaultLoadFlags = aDefaultLoadFlags;
> 
>     // Tell the load group to set these flags all requests in the group
>     if (mLoadGroup) {
>         mLoadGroup->SetDefaultLoadFlags(aDefaultLoadFlags);
>     } else {
>@@ -8686,18 +8726,20 @@ nsDocShell::InternalLoad(nsIURI * aURI,
>     int16_t shouldLoad = nsIContentPolicy::ACCEPT;
>     uint32_t contentType;
>     bool isNewDocShell = false;
>     bool isTargetTopLevelDocShell = false;
>     nsCOMPtr<nsIDocShell> targetDocShell;
>     if (aWindowTarget && *aWindowTarget) {
>         // Locate the target DocShell.
>         nsCOMPtr<nsIDocShellTreeItem> targetItem;
>-        FindItemWithName(aWindowTarget, nullptr, this,
>-                         getter_AddRefs(targetItem));
>+        if (FindItemWithName(aWindowTarget, nullptr, this,
>+               getter_AddRefs(targetItem)) == NS_ERROR_DOM_INVALID_ACCESS_ERR) {
>+            return NS_ERROR_DOM_INVALID_ACCESS_ERR;
>+        }
> 
>         targetDocShell = do_QueryInterface(targetItem);
>         // If the targetDocShell doesn't exist, then this is a new docShell
>         // and we should consider this a TYPE_DOCUMENT load
>         isNewDocShell = !targetDocShell;
> 
>         // If the targetDocShell and the rootDocShell are the same, then the
>         // targetDocShell is the top level document and hence we should
>@@ -8814,29 +8856,27 @@ nsDocShell::InternalLoad(nsIURI * aURI,
>     if (aWindowTarget && *aWindowTarget) {
>         // We've already done our owner-inheriting.  Mask out that bit, so we
>         // don't try inheriting an owner from the target window if we came up
>         // with a null owner above.
>         aFlags = aFlags & ~INTERNAL_LOAD_FLAGS_INHERIT_OWNER;
>         
>         bool isNewWindow = false;
>         if (!targetDocShell) {
>-            // If the docshell's document is sandboxed and was trying to
>-            // navigate/load a frame it wasn't allowed to access, the
>-            // FindItemWithName above will have returned null for the target
>-            // item - we don't want to actually open a new window in this case
>-            // though. Check if we are sandboxed and bail out here if so.
>+            // If the docshell's document is sandboxed, only open a new window
>+            // if the document's SANDBOXED_AUXILLARY_NAVIGATION flag is not set.
>+            // (i.e. if allow-popups is specified)
>             NS_ENSURE_TRUE(mContentViewer, NS_ERROR_FAILURE);
>             nsIDocument* doc = mContentViewer->GetDocument();
>             uint32_t sandboxFlags = 0;
> 
>             if (doc) {
>                 sandboxFlags = doc->GetSandboxFlags();
>-                if (sandboxFlags & SANDBOXED_NAVIGATION) {
>-                    return NS_ERROR_FAILURE;
>+                if (sandboxFlags & SANDBOXED_AUXILIARY_NAVIGATION) {
>+                    return NS_ERROR_DOM_INVALID_ACCESS_ERR;
>                 }
>             }
> 
>             nsCOMPtr<nsPIDOMWindow> win =
>                 do_GetInterface(GetAsSupports(this));
>             NS_ENSURE_TRUE(win, NS_ERROR_NOT_AVAILABLE);
> 
>             nsDependentString name(aWindowTarget);
>diff --git a/docshell/base/nsDocShell.h b/docshell/base/nsDocShell.h
>--- a/docshell/base/nsDocShell.h
>+++ b/docshell/base/nsDocShell.h
>@@ -764,16 +764,17 @@ protected:
>     int32_t                    mItemType;
> 
>     // Index into the SHTransaction list, indicating the previous and current
>     // transaction at the time that this DocShell begins to load
>     int32_t                    mPreviousTransIndex;
>     int32_t                    mLoadedTransIndex;
> 
>     uint32_t                   mSandboxFlags;
>+    nsWeakPtr                  mOnePermittedSandboxedNavigator;
> 
>     // mFullscreenAllowed stores how we determine whether fullscreen is allowed
>     // when GetFullscreenAllowed() is called. Fullscreen is allowed in a
>     // docshell when all containing iframes have the allowfullscreen
>     // attribute set to true. When mFullscreenAllowed is CHECK_ATTRIBUTES
>     // we check this docshell's containing frame for the allowfullscreen
>     // attribute, and recurse onto the parent docshell to ensure all containing
>     // frames also have the allowfullscreen attribute. If we find an ancestor
>@@ -870,16 +871,20 @@ 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);
> 
>+    // Check whether accessing item is sandboxed from the target item.
>+    static bool IsSandboxedFrom(nsIDocShellTreeItem* aTargetItem,
>+                                nsIDocShellTreeItem* aAccessingItem);
>+
> #ifdef DEBUG
>     // We're counting the number of |nsDocShells| to help find leaks
>     static unsigned long gNumberOfDocShells;
> #endif /* DEBUG */
> 
> public:
>     class InterfaceRequestorProxy : public nsIInterfaceRequestor {
>     public:
>diff --git a/docshell/base/nsIDocShell.idl b/docshell/base/nsIDocShell.idl
>--- a/docshell/base/nsIDocShell.idl
>+++ b/docshell/base/nsIDocShell.idl
>@@ -39,17 +39,17 @@ interface nsIDOMStorage;
> interface nsIPrincipal;
> interface nsIWebBrowserPrint;
> interface nsIVariant;
> interface nsIPrivacyTransitionObserver;
> interface nsIReflowObserver;
> 
> typedef unsigned long nsLoadFlags;
> 
>-[scriptable, builtinclass, uuid(5f4d82fc-3220-4f7e-9b00-626f1033318a)]
>+[scriptable, builtinclass, uuid(4ca172c3-67bf-4e6d-89a3-cbfb929c370d)]
> interface nsIDocShell : nsIDocShellTreeItem
> {
>   /**
>    * Loads a given URI.  This will give priority to loading the requested URI
>    * in the object implementing	this interface.  If it can't be loaded here
>    * however, the URL dispatcher will go through its normal process of content
>    * loading.
>    *
>@@ -795,16 +795,23 @@ interface nsIDocShell : nsIDocShellTreeI
>    * loaded.
>    * The sandbox flags of a document depend on the sandbox flags on its
>    * docshell and of its parent document, if any.
>    * See nsSandboxFlags.h for the possible flags.
>    */
>   attribute unsigned long sandboxFlags;
> 
>   /**
>+   * When a new browsing context is opened by a sandboxed document, it needs to
>+   * keep track of the browsing context that opened it, so that it can be
>+   * navigated by it.  This is the "one permitted sandboxed navigator".
>+   */
>+  attribute nsIDocShell onePermittedSandboxedNavigator;
>+
>+  /**
>    * This member variable determines whether a document has Mixed Active Content that
>    * was initially blocked from loading, but the user has choosen to override the
>    * block and allow the content to load. mMixedContentChannel is set to the document's
>    * channel when the user allows mixed content. The nsMixedContentBlocker content policy
>    * checks if the document's root channel matches the mMixedContentChannel.  If it matches,
>    * then Mixed Content is loaded.  If it does match, mixed content is blocked.
>    *
>    * A match implies that there is definitely mixed active content on a page that was
>diff --git a/embedding/components/windowwatcher/src/nsWindowWatcher.cpp b/embedding/components/windowwatcher/src/nsWindowWatcher.cpp
>--- a/embedding/components/windowwatcher/src/nsWindowWatcher.cpp
>+++ b/embedding/components/windowwatcher/src/nsWindowWatcher.cpp
>@@ -470,17 +470,20 @@ nsWindowWatcher::OpenWindowInternal(nsID
>   if (aFeatures) {
>     features.Assign(aFeatures);
>     featuresSpecified = true;
>     features.StripWhitespace();
>   }
> 
>   // try to find an extant window with the given name
>   nsCOMPtr<nsIDOMWindow> foundWindow;
>-  SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow));
>+  if (SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow)) ==
>+      NS_ERROR_DOM_INVALID_ACCESS_ERR) {
>+    return NS_ERROR_DOM_INVALID_ACCESS_ERR;
>+  }
>   GetWindowTreeItem(foundWindow, getter_AddRefs(newDocShellItem));
> 
>   // no extant window? make a new one.
> 
>   // If no parent, consider it chrome.
>   bool hasChromeParent = true;
>   if (aParent) {
>     // Check if the parent document has chrome privileges.
>@@ -538,31 +541,36 @@ nsWindowWatcher::OpenWindowInternal(nsID
>     // the initial about:blank document to be system, so that the subsequent XUL
>     // load can reuse the inner window and avoid blowing away expandos. As such,
>     // we decide whether to load with the principal of the caller or of the parent
>     // based on whether the docshell type is chrome or content.
> 
>     callerContextGuard.Push(cx);
>   }
> 
>+  uint32_t activeDocsSandboxFlags = 0;
>   if (!newDocShellItem) {
>     // We're going to either open up a new window ourselves or ask a
>     // nsIWindowProvider for one.  In either case, we'll want to set the right
>     // name on it.
>     windowNeedsName = true;
> 
>-    // If the parent trying to open a new window is sandboxed,
>-    // this is not allowed and we fail here.
>+    // If the parent trying to open a new window is sandboxed
>+    // without 'allow-popups', this is not allowed and we fail here.
>     if (aParent) {
>       nsCOMPtr<nsIDOMDocument> domdoc;
>       aParent->GetDocument(getter_AddRefs(domdoc));
>       nsCOMPtr<nsIDocument> doc = do_QueryInterface(domdoc);
> 
>-      if (doc && (doc->GetSandboxFlags() & SANDBOXED_NAVIGATION)) {
>-        return NS_ERROR_FAILURE;
>+      if (doc) {
>+        // Save sandbox flags for copying to new browsing context (docShell).
>+        activeDocsSandboxFlags = doc->GetSandboxFlags();
>+        if (activeDocsSandboxFlags & SANDBOXED_AUXILIARY_NAVIGATION) {
>+          return NS_ERROR_DOM_INVALID_ACCESS_ERR;
>+        }
>       }
>     }
> 
>     // Now check whether it's ok to ask a window provider for a window.  Don't
>     // do it if we're opening a dialog or if our parent is a chrome window or
>     // if we're opening something that has modal, dialog, or chrome flags set.
>     nsCOMPtr<nsIDOMChromeWindow> chromeWin = do_QueryInterface(aParent);
>     if (!aDialog && !chromeWin &&
>@@ -704,16 +712,26 @@ nsWindowWatcher::OpenWindowInternal(nsID
>   }
> 
>   // better have a window to use by this point
>   if (!newDocShellItem)
>     return rv;
> 
>   nsCOMPtr<nsIDocShell> newDocShell(do_QueryInterface(newDocShellItem));
>   NS_ENSURE_TRUE(newDocShell, NS_ERROR_UNEXPECTED);
>+
>+  // Set up sandboxing attributes if the window is new.
>+  // The flags can only be non-zero for new windows.
>+  if (activeDocsSandboxFlags != 0) {
>+    newDocShell->SetSandboxFlags(activeDocsSandboxFlags);
>+    nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aParent);
>+    if (window) {
>+      newDocShell->SetOnePermittedSandboxedNavigator(window->GetDocShell());
>+    }
>+  }
>   
>   rv = ReadyOpenedDocShellItem(newDocShellItem, aParent, windowIsNew, _retval);
>   if (NS_FAILED(rv))
>     return rv;
> 
>   /* disable persistence of size/position in popups (determined by
>      determining whether the features parameter specifies width or height
>      in any way). We consider any overriding of the window's size or position
>@@ -1294,37 +1312,38 @@ nsWindowWatcher::GetChromeForWindow(nsID
> NS_IMETHODIMP
> nsWindowWatcher::GetWindowByName(const PRUnichar *aTargetName, 
>                                  nsIDOMWindow *aCurrentWindow,
>                                  nsIDOMWindow **aResult)
> {
>   if (!aResult) {
>     return NS_ERROR_INVALID_ARG;
>   }
>+  nsresult rv;
> 
>   *aResult = nullptr;
> 
>   nsCOMPtr<nsIDocShellTreeItem> treeItem;
> 
>   nsCOMPtr<nsIDocShellTreeItem> startItem;
>   GetWindowTreeItem(aCurrentWindow, getter_AddRefs(startItem));
>   if (startItem) {
>     // Note: original requestor is null here, per idl comments
>-    startItem->FindItemWithName(aTargetName, nullptr, nullptr,
>+    rv = startItem->FindItemWithName(aTargetName, nullptr, nullptr,
>                                 getter_AddRefs(treeItem));
>   }
>   else {
>     // Note: original requestor is null here, per idl comments
>-    FindItemWithName(aTargetName, nullptr, nullptr, getter_AddRefs(treeItem));
>+    rv = FindItemWithName(aTargetName, nullptr, nullptr, getter_AddRefs(treeItem));
>   }
> 
>   nsCOMPtr<nsIDOMWindow> domWindow = do_GetInterface(treeItem);
>   domWindow.swap(*aResult);
> 
>-  return NS_OK;
>+  return rv;
> }
> 
> bool
> nsWindowWatcher::AddEnumerator(nsWatcherWindowEnumerator* inEnumerator)
> {
>   // (requires a lock; assumes it's called by someone holding the lock)
>   return mEnumeratorList.AppendElement(inEnumerator) != nullptr;
> }
>@@ -1726,37 +1745,38 @@ nsWindowWatcher::GetCallerTreeItem(nsIDo
> }
> 
> nsresult
> nsWindowWatcher::SafeGetWindowByName(const nsAString& aName,
>                                      nsIDOMWindow* aCurrentWindow,
>                                      nsIDOMWindow** aResult)
> {
>   *aResult = nullptr;
>+  nsresult rv;
>   
>   nsCOMPtr<nsIDocShellTreeItem> startItem;
>   GetWindowTreeItem(aCurrentWindow, getter_AddRefs(startItem));
> 
>   nsCOMPtr<nsIDocShellTreeItem> callerItem = GetCallerTreeItem(startItem);
> 
>   const nsAFlatString& flatName = PromiseFlatString(aName);
> 
>   nsCOMPtr<nsIDocShellTreeItem> foundItem;
>   if (startItem) {
>-    startItem->FindItemWithName(flatName.get(), nullptr, callerItem,
>+    rv = startItem->FindItemWithName(flatName.get(), nullptr, callerItem,
>                                 getter_AddRefs(foundItem));
>   }
>   else {
>-    FindItemWithName(flatName.get(), nullptr, callerItem,
>+    rv = FindItemWithName(flatName.get(), nullptr, callerItem,
>                      getter_AddRefs(foundItem));
>   }
> 
>   nsCOMPtr<nsIDOMWindow> foundWin = do_GetInterface(foundItem);
>   foundWin.swap(*aResult);
>-  return NS_OK;
>+  return rv;
> }
> 
> /* Fetch the nsIDOMWindow corresponding to the given nsIDocShellTreeItem.
>    This forces the creation of a script context, if one has not already
>    been created. Note it also sets the window's opener to the parent,
>    if applicable -- because it's just convenient, that's all. null aParent
>    is acceptable. */
> nsresult
Attachment #805554 - Flags: review?(bugs) → review+
Comment on attachment 798592 [details] [diff] [review]
allow-popups tests

(In reply to Olli Pettay [:smaug] from comment #69)
> Comment on attachment 805554 [details] [diff] [review]
> allow-popups code changes

Thanks Olli, that's great.

You asked me to re-request the review for the tests once the patch was r+.
Attachment #798592 - Flags: review?(bugs)
Comment on attachment 798592 [details] [diff] [review]
allow-popups tests


>+function testAttempted() {
>+  attemptedTests++;
>+  if (attemptedTests == totalTestsToAttempt) {
>+    // Make sure all tests have had a chance to complete.
>+    setTimeout(function() {finish();}, 1000);
>+  }
This looks like very failure prone. Why 1000ms? Why not 10 or 3000?
Should just make sure all the tests had chance to complete and call finish(), without timeout.

In the earlier comments you talked about some exception related to modal dialogs.
Are there timeouts for that? We should just figure out why the exception is happening and perhaps fix something.
Could you perhaps write minimal testcase and file a bug about it?


But looking still the rest of the patch. (not exactly easy to review.)
Attachment #798592 - Flags: review?(bugs) → review-
Also, usually it is best to try to not do non-relevant white space fixes in a patch which is huge even without them.
But ok, other than the setTimeout, looks ok to me.
(In reply to Olli Pettay [:smaug] from comment #71)
> Comment on attachment 798592 [details] [diff] [review]
> allow-popups tests

Thanks again for taking the time on this Olli.

> >+function testAttempted() {
> >+  attemptedTests++;
> >+  if (attemptedTests == totalTestsToAttempt) {
> >+    // Make sure all tests have had a chance to complete.
> >+    setTimeout(function() {finish();}, 1000);
> >+  }
> This looks like very failure prone. Why 1000ms? Why not 10 or 3000?
> Should just make sure all the tests had chance to complete and call
> finish(), without timeout.

I made these changes because there are tests that only fail if they go wrong.
These tend to be ones where a link is clicked within an iframe that gets blocked.
I can't find a way to detect if the test has finished except when it fails.

As I was writing the tests first, I noticed that sometimes these tests would not report a failure when they should because the overall test was finishing before they had the chance.
This was happening for some of the original tests as well.
So, I changed it so that all the tests would increment attemptedTests, either when they report a pass for normal tests, or just before the last line before we lose track (normally a click of some sort) for the "fails if bad" tests.
Once all the tests have at least got that far, it waits for 1000ms, to make sure all of the "fails if bad" tests have had a chance to fail.

You're right it is an arbitrary number, but it should give enough time for the navigations to take place. I didn't want to make it too big and delay the overall test suites.
Either way it is better than finishing the tests straight after all the ones that pass have reported.
Of course if there is some other way that we can detect that the navigation has been blocked, then we could log a pass and get rid of this issue.

> In the earlier comments you talked about some exception related to modal
> dialogs.
> Are there timeouts for that? We should just figure out why the exception is
> happening and perhaps fix something.
> Could you perhaps write minimal testcase and file a bug about it?
 
I think these were just failing on Android and B2G because they don't support it, so I split those tests out and ignored them on these platforms.

(In reply to Olli Pettay [:smaug] from comment #72)
> Also, usually it is best to try to not do non-relevant white space fixes in
> a patch which is huge even without them.

Sorry, I'll make sure I do this sort of thing as a separate patch in future, to make them easier to review.
So what happens if 1000ms is not enough?
(note on debug builds in cases when GC/CC happen to run at odd times and if the testing machine has
low memory, pause times can in theory be quite bad.)
(In reply to Olli Pettay [:smaug] from comment #75)
> So what happens if 1000ms is not enough?
> (note on debug builds in cases when GC/CC happen to run at odd times and if
> the testing machine has
> low memory, pause times can in theory be quite bad.)

Then there is a potential that a "fails if bad" test won't have had time to report a failure.

However, the previous approach just finished when all the tests that reported a pass had finished, so the above scenario was even more likely to happen.

This way we know that all the tests have at least had chance to get to their critical step and we then give them a little more time to report a failure.
(In reply to Bob Owen from comment #74) 
> As I was writing the tests first, I noticed that sometimes these tests would
> not report a failure when they should because the overall test was finishing
> before they had the chance.
Well, couldn't we just ensure that we get all the reports before calling the Finish()?
(In reply to Olli Pettay [:smaug] from comment #77)
> (In reply to Bob Owen from comment #74) 
> > As I was writing the tests first, I noticed that sometimes these tests would
> > not report a failure when they should because the overall test was finishing
> > before they had the chance.
> Well, couldn't we just ensure that we get all the reports before calling the
> Finish()?

No, because they only report if they fail, which should only happen if someone accidentally introduces a bug.

If they "pass" we don't know about it because we can't detect the block to navigation.
So the best we can do is make sure they have at least got to the navigation step and then give them a bit of time to see if the navigation is not blocked, which would be a failure.
Hi Olli, do you need anything further to my reply in comment 78?

Should I put the tests back for review?
Flags: needinfo?(bugs)
Hmm, perhaps just ask review again, and I'll check it again.
Can you push the patch with tests to tryserver or should I?
Flags: needinfo?(bugs)
Comment on attachment 798592 [details] [diff] [review]
allow-popups tests

Thanks Olli.

I did a try run back in Comment 65:
https://tbpl.mozilla.org/?tree=Try&rev=ecb13dded996

I can re-merge from mozilla-central and do another one if you like, but it might have to wait until tomorrow.
Attachment #798592 - Flags: review- → review?(bugs)
Comment on attachment 798592 [details] [diff] [review]
allow-popups tests

Ok, if you could do that, and could you re-trigger these tests couple of times on try.
Attachment #798592 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #82)
> Comment on attachment 798592 [details] [diff] [review]
> allow-popups tests
> 
> Ok, if you could do that, and could you re-trigger these tests couple of
> times on try.

I'll get onto it as soon as I get back from work tomorrow.
New version of the tests patch due to the removal of Makefile.in.
Changes now added to mochitest.ini for new tests and supporting files.
I assume I don't have to make any other changes as the new tests seem to be running fine.

New try run:
https://tbpl.mozilla.org/?tree=Try&rev=39d23888c05d

Starting to run test-suites that run the sandbox tests three times.

Try run just for B2G:
https://tbpl.mozilla.org/?tree=Try&rev=c479e3453e69

In the B2G.json file test_iframe_sandbox_popups_inheritance.html was popup not popups and so was running and failing for B2G.
Attachment #798592 - Attachment is obsolete: true
Attachment #816111 - Flags: review?(bugs)
Comment on attachment 816111 [details] [diff] [review]
allow-popups tests

Did you trigger the tests several times on try.
Please do before landing.
Attachment #816111 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #85)
> Comment on attachment 816111 [details] [diff] [review]
> allow-popups tests
> 
> Did you trigger the tests several times on try.
> Please do before landing.


Thanks Olli,


I only ran the tests three times per platform / build type.

So I'll kick off some more runs before landing.
Note: the "Fix line endings on test files." patch (bug766282dos2unix.patch) should be applied after "allow-popups tests" (bug766282tests.patch).
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/fce96290655d
https://hg.mozilla.org/integration/mozilla-inbound/rev/12f30ee31b2e

I rolled the whitespace fixups into the tests patch. Made more sense to me to do it that way :)
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fce96290655d
https://hg.mozilla.org/mozilla-central/rev/12f30ee31b2e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 939642
You need to log in before you can comment on or make changes to this bug.