1268810 - [Presentation WebAPI] set sandboxed auxiliary navigation browsing context flag on receiving browsing context

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

We need to set sandboxed auxiliary navigation browsing context flag to prevent presented content opening popup window.

Comment 1

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add SANDBOXED_AUXILIARY_NAVIGATION flag to receiver page

First version.

Comment 2

[Kershaw Chang [:kershaw]]

Attachment: Part2: Test case

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8763028 [details] [diff] [review]
Part1: Add SANDBOXED_AUXILIARY_NAVIGATION flag to  receiver page

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

::: dom/base/nsFrameLoader.cpp
@@ +2137,5 @@
> +  HTMLIFrameElement* iframe = HTMLIFrameElement::FromContent(mOwnerContent);
> +  if (iframe) {
> +    sandboxFlags = iframe->GetSandboxFlags();
> +  }
> +  ApplySandboxFlags(sandboxFlags);

Why we need to move this code? Please add comment for it if there is legitimate reason.

@@ +3014,5 @@
> +    nsAutoString presentationURL;
> +    nsContentUtils::GetPresentationURL(mDocShell, presentationURL);
> +    if (!presentationURL.IsEmpty()) {
> +      sandboxFlags |= SANDBOXED_AUXILIARY_NAVIGATION;
> +    }

Please add a comment for it, or leave a reference to spec here.

::: dom/ipc/TabChild.cpp
@@ +894,5 @@
>    nsDocShell::Cast(docShell)->SetOriginAttributes(OriginAttributesRef());
> +
> +  if (!PresentationURL().IsEmpty()) {
> +    docShell->SetSandboxFlags(SANDBOXED_AUXILIARY_NAVIGATION);
> +  }

comment here, too.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8763029 [details] [diff] [review]
Part2: Test case

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

::: dom/presentation/tests/mochitest/mochitest.ini
@@ +50,5 @@
>  skip-if = (e10s || toolkit == 'gonk' || toolkit == 'android') # Bug 1129785
> +[test_presentation_receiver_auxiliary_navigation_inproc.html]
> +skip-if = (e10s || toolkit == 'gonk' || toolkit == 'android') # Bug 1129785
> +[test_presentation_receiver_auxiliary_navigation_oop.html]
> +skip-if = (e10s || toolkit == 'gonk' || toolkit == 'android') # Bug 1129785

we can probably run this test on Android as well. please try it.

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #3)
> Comment on attachment 8763028 [details] [diff] [review]
> Part1: Add SANDBOXED_AUXILIARY_NAVIGATION flag to  receiver page
> 
> Review of attachment 8763028 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsFrameLoader.cpp
> @@ +2137,5 @@
> > +  HTMLIFrameElement* iframe = HTMLIFrameElement::FromContent(mOwnerContent);
> > +  if (iframe) {
> > +    sandboxFlags = iframe->GetSandboxFlags();
> > +  }
> > +  ApplySandboxFlags(sandboxFlags);
> 
> Why we need to move this code? Please add comment for it if there is
> legitimate reason.
> 

We have to do mDocShell->SetFrameType first for setting the mozbrowser boundary, or we will get a wrong top frame element in nsContentUtils::GetPresentationURL.

> @@ +3014,5 @@
> > +    nsAutoString presentationURL;
> > +    nsContentUtils::GetPresentationURL(mDocShell, presentationURL);
> > +    if (!presentationURL.IsEmpty()) {
> > +      sandboxFlags |= SANDBOXED_AUXILIARY_NAVIGATION;
> > +    }
> 
> Please add a comment for it, or leave a reference to spec here.
> 

Will do.

> ::: dom/ipc/TabChild.cpp
> @@ +894,5 @@
> >    nsDocShell::Cast(docShell)->SetOriginAttributes(OriginAttributesRef());
> > +
> > +  if (!PresentationURL().IsEmpty()) {
> > +    docShell->SetSandboxFlags(SANDBOXED_AUXILIARY_NAVIGATION);
> > +  }
> 
> comment here, too.

Will do. Thanks.

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add SANDBOXED_AUXILIARY_NAVIGATION flag to receiver page - v2

Add some comments.

Comment 7

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #4)
> Comment on attachment 8763029 [details] [diff] [review]
> Part2: Test case
> 
> Review of attachment 8763029 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/presentation/tests/mochitest/mochitest.ini
> @@ +50,5 @@
> >  skip-if = (e10s || toolkit == 'gonk' || toolkit == 'android') # Bug 1129785
> > +[test_presentation_receiver_auxiliary_navigation_inproc.html]
> > +skip-if = (e10s || toolkit == 'gonk' || toolkit == 'android') # Bug 1129785
> > +[test_presentation_receiver_auxiliary_navigation_oop.html]
> > +skip-if = (e10s || toolkit == 'gonk' || toolkit == 'android') # Bug 1129785
> 
> we can probably run this test on Android as well. please try it.

Let's see if this can pass on Android.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae7106e72500

Comment 8

[Kershaw Chang [:kershaw]]

Attachment: Part2: Test case -v2

Enable tests on Android.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8763752 [details] [diff] [review]
Part2: Test case -v2

Not about this bug, but presentation code in general. I wonder if we should add a new module for it and have couple of peers for reviewing and such?

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Part1: Add SANDBOXED_AUXILIARY_NAVIGATION flag to receiver page, r=smaug

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: Part2: Test case, r=smaug

Comment 12

[Kershaw Chang [:kershaw]]

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #9)
> Comment on attachment 8763752 [details] [diff] [review]
> Part2: Test case -v2
> 
> Not about this bug, but presentation code in general. I wonder if we should
> add a new module for it and have couple of peers for reviewing and such?

Sounds a good idea.

I think S.C. should be the peer or perhaps the owner. :)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29e67b27dae3
Set SANDBOXED_AUXILIARY_NAVIGATION flag to receiver page, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/79918d9ceb0b
Test case, r=smaug

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Kershaw Chang [:kershaw] from comment #12)
> (In reply to Olli Pettay [:smaug] (high review load, please consider other
> reviewers) from comment #9)
> > Comment on attachment 8763752 [details] [diff] [review]
> > Part2: Test case -v2
> > 
> > Not about this bug, but presentation code in general. I wonder if we should
> > add a new module for it and have couple of peers for reviewing and such?
> 
> Sounds a good idea.
> 
> I think S.C. should be the peer or perhaps the owner. :)

I'm ok with it since I've been doing drive-by review for a while.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29e67b27dae3
https://hg.mozilla.org/mozilla-central/rev/79918d9ceb0b

Comment 16

[Kershaw Chang [:kershaw]]

Attachment: [b2g_48_v2_6] Part1: Add SANDBOXED_AUXILIARY_NAVIGATION flag to receiver page, r=smaug

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Presentation API
User impact if declined: Receiver page can do window.open().
Testing completed: with automation test
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: N/A

Comment 17

[Kershaw Chang [:kershaw]]

Josh,

Please help to approve. Thanks.

Comment 18

[Josh Cheng [:josh]]

Comment on attachment 8765806 [details] [diff] [review]
[b2g_48_v2_6] Part1: Add SANDBOXED_AUXILIARY_NAVIGATION flag to receiver page, r=smaug

Approve for TV 2.6

Comment 19

[Josh Cheng [:josh]]

Comment on attachment 8764101 [details] [diff] [review]
Part2: Test case, r=smaug

Approve for TV 2.6

Comment 20

[Kershaw Chang [:kershaw]]

Gary, please help to merge the pull request.

Thanks.

https://github.com/mozilla-b2g/gecko-b2g/pull/29

Comment 21

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/e81fd19f9e908939f9476b95b85d09e155b4aaea