Closed Bug 1268758 Opened 4 years ago Closed 3 years ago

[Presentation WebAPI] implement allow-presentation sandboxing flag

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
b2g-v2.6 --- fixed
firefox50 --- fixed

People

(Reporter: schien, Assigned: kershaw)

References

()

Details

(Whiteboard: [ft:conndevices], btpp-backlog)

Attachments

(3 files, 3 obsolete files)

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.
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.
Whiteboard: btpp-backlog
(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?
Flags: needinfo?(bugs)
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.)
Flags: needinfo?(bugs)
Whiteboard: btpp-backlog → [ft:conndevices], btpp-backlog
No longer blocks: 1259360
Assignee: nobody → kechang
Attachment #8767081 - Flags: feedback?(schien)
Attached patch Part2: Test case (obsolete) — Splinter Review
Attachment #8767082 - Flags: feedback?(schien)
Attachment #8767082 - Flags: feedback?(schien) → feedback+
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
Attachment #8767081 - Flags: feedback?(schien) → feedback+
(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?
(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.
Attachment #8767081 - Attachment is obsolete: true
Attachment #8767639 - Flags: review?(bugs)
Attachment #8767082 - Flags: review?(bugs)
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.
Attachment #8767639 - Flags: review?(bugs) → review+
Attachment #8767082 - Flags: review?(bugs) → review+
Rebase
Attachment #8767082 - Attachment is obsolete: true
Attachment #8767639 - Attachment is obsolete: true
Attachment #8770885 - Flags: review+
Attachment #8770886 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/54bc921dd236
https://hg.mozilla.org/mozilla-central/rev/5c73f49fee35
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
Attachment #8779197 - Flags: approval-mozilla-b2g48?
(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.
Flags: needinfo?(jocheng)
Comment on attachment 8779197 [details] [review]
pull request for tv 2.6

Approve for TV 2.6
Flags: needinfo?(jocheng)
Attachment #8779197 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.