1268758 - [Presentation WebAPI] implement allow-presentation sandboxing flag

Status: RESOLVED FIXED

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

Description

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

Presentation is disabled in a browsing context when the document object's active sandboxing flag set does not have the sandboxed presentation browsing context flag set. 

The detailed behavior of disabling Presentation API is discussed in https://github.com/w3c/presentation-api/issues/288.

Comment 1

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

Need to add "allow-presentation" in GkAtomList and add a SANDBOXED_PRESENTATION flag in
https://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentUtils.cpp#1353.

Then, check the sandboxing flag from document to deny access like in https://dxr.mozilla.org/mozilla-central/source/dom/base/ScreenOrientation.cpp#465.

Comment 2

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

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #0)
> Presentation is disabled in a browsing context when the document object's
> active sandboxing flag set does not have the sandboxed presentation browsing
> context flag set. 
> 
> The detailed behavior of disabling Presentation API is discussed in
> https://github.com/w3c/presentation-api/issues/288.

In latest editor draft, the detailed behavior is defined as following:
1. navigator.presentation is always accessible in sandbox
2. PresentationRequest.start / .reconnect / .getAvailability will be reject with SecurityError.

I'm ok with this unified error detection, @smaug how do you think?

Comment 3

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

I guess that is fine. Don't know why that a bit more complicated approach was taken (and why not just have null navigator.presentation and throwing from creating new PresentationRequest, but if the wg thinks that is ok, fine.)

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

FWIW, Google is starting work on this:

  https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/KoasIWagGsk

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: Part1: Implement allow-presentation sandbox flag

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Part2: Test case

Comment 7

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

Comment on attachment 8767081 [details] [diff] [review]
Part1: Implement allow-presentation sandbox flag

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

::: dom/presentation/Presentation.cpp
@@ -3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -#include <ctype.h>

Looks like this header is added in Bug 1276927 to solve build issue. You'll need to double check to avoid regression.

@@ +57,5 @@
>    if (IsInPresentedContent()) {
>      return;
>    }
>  
> +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();

check if "GetOwner()" return null as well.

::: dom/presentation/PresentationRequest.cpp
@@ +103,5 @@
>      aRv.Throw(rv);
>      return nullptr;
>    }
>  
> +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();

ditto

@@ +105,5 @@
>    }
>  
> +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
> +  if (NS_WARN_IF(!doc)) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);

throw NS_ERROR_FAILURE should be enough for all the unknown error.

@@ +160,5 @@
>      aRv.Throw(NS_ERROR_UNEXPECTED);
>      return nullptr;
>    }
>  
> +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();

ditto

Comment 8

[Kershaw Chang [:kershaw]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #7)
> Comment on attachment 8767081 [details] [diff] [review]
> Part1: Implement allow-presentation sandbox flag
> 
> Review of attachment 8767081 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/presentation/Presentation.cpp
> @@ -3,5 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > -#include <ctype.h>
> 
> Looks like this header is added in Bug 1276927 to solve build issue. You'll
> need to double check to avoid regression.
> 

I see. I will add this header back in next version since I don't have an environment to build b2g.
Once I verify this can be removed, will file another bug to do this.

> @@ +57,5 @@
> >    if (IsInPresentedContent()) {
> >      return;
> >    }
> >  
> > +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
> 
> check if "GetOwner()" return null as well.
> 

Will do. Thanks.

> ::: dom/presentation/PresentationRequest.cpp
> @@ +103,5 @@
> >      aRv.Throw(rv);
> >      return nullptr;
> >    }
> >  
> > +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
> 
> ditto
> 

GetOwner() has been called at the begin of this function to get the nsIGlobalObject object.
I think we don't have to check again?

Comment 9

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

(In reply to Kershaw Chang [:kershaw] from comment #8)
> (In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment
> #7)
> > ::: dom/presentation/PresentationRequest.cpp
> > @@ +103,5 @@
> > >      aRv.Throw(rv);
> > >      return nullptr;
> > >    }
> > >  
> > > +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
> > 
> > ditto
> > 
> 
> GetOwner() has been called at the begin of this function to get the
> nsIGlobalObject object.
> I think we don't have to check again?

Got it, didn't notice that in diff file.

Comment 10

[Kershaw Chang [:kershaw]]

Attachment: Part1: Implement allow-presentation sandbox flag - v2

Comment 11

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

Comment on attachment 8767639 [details] [diff] [review]
Part1: Implement allow-presentation sandbox flag - v2

Looks like we don't support reconnect yet. That would need flag handling too.

Comment 12

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

I did file a spec bug: https://github.com/w3c/presentation-api/issues/323

Comment 13

[Kershaw Chang [:kershaw]]

Attachment: Part1: Implement allow-presentation sandbox flag, r=smaug

Rebase

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: Test case, r=smaug

Comment 15

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ea4f733a657

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54bc921dd236
Part1: Implement allow-presentation sandboxing flag, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c73f49fee35
Part2: Test case, r=smaug

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54bc921dd236
https://hg.mozilla.org/mozilla-central/rev/5c73f49fee35

Comment 18

[Kershaw Chang [:kershaw]]

Attachment: pull request for tv 2.6

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: Not be aligned with spec.
Testing completed: Local test.
Risk to taking this patch (and alternatives if risky): N/A
String or UUID changes made by this patch: N/A

Comment 19

[Kershaw Chang [:kershaw]]

(In reply to Kershaw Chang [:kershaw] from comment #18)
> Created attachment 8779197 [details] [review]
> pull request for tv 2.6
> 
> 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: Not be aligned with spec.
> Testing completed: Local test.
> Risk to taking this patch (and alternatives if risky): N/A
> String or UUID changes made by this patch: N/A

Josh,

Please help to approve this. Thanks.

Comment 20

[Josh Cheng [:josh]]

Comment on attachment 8779197 [details] [review]
pull request for tv 2.6

Approve for TV 2.6

Comment 21

[Gary Chen [:xeonchen]]

https://github.com/mozilla-b2g/gecko-b2g/commit/9db666adbe8066750ecd2c2f15184560a4ed2897