Closed Bug 1415444 Opened 7 years ago Closed 7 years ago

Add a flag to know whether the website has been interacted by specific user gestures

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
Other Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(2 files)

Bug 1415444 - part1 : add flag to record whether the document has been activated by specific user gestures.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1415444 - part2 : add log.
59 bytes, text/x-review-board-request
smaug
: review+
Details 
We'll plan to unified the autoplay media behavior [1], and want to reference the implementation of Chrome. [2]

The basic idea is that if a website has interacted with specific user gestures (now we temporarily restrict it to "click", it could be more kinds in the future), then we would allow the autoplay on this website. Otherwise, we would reject the play request.

But there are some other conditions could allow websites to autoplay, 
(1) if navigating in the same eTLD+1, the new website would also have permission
(2) when website has multiple frames, if one of them has got permission, it might also give another frame permission

---

Therefore, we want to create a new flag to record whether the website has been interacted by specific user gestures. If user clicks on the site, the flag would be set to true.
 
When user navigate in the same eTLD+1, and the flag of previous page is true, then
the flag of new visited site would also be true.

If the site has multiple iframes and then we click the inner iframe, the flag of iframe and its entire parent chain would be set to true.

When the top frame is clicked, the flag of its child frame would be set to true only when the child frame has "feature policy" flag.

About the policy of the iframe, see more details in "Delegating user gesture" in [2].

---

[1] https://docs.google.com/presentation/d/1_-vuAJSM8NShA53sT9NV41E2t7Pa80bxagRis7DiJXA/edit#slide=id.g2a813cc79f_0_16

[2] https://docs.google.com/document/d/1EH7qZatVnTXsBGvQc_53R97Z0xqm6zRblKg3eVmNp30/edit#heading=h.k7qqsf3fbmme
Chrome's implementation is to put the flag on Frame [1], and then they check that flag to decide whether allows autoplay. [2]

---

Hi, smaug, stone,
Here I have some questions, could you give me some suggestions?

(1) where is the proper place to put this flag? maybe document or inner window?
(2) do we have a place where could handle all kinds of event and we could just do the check on there?
    or we need to add the event listener to monitor for specific events?
    if so, where is the good place to put this event listener?
(3) when navigating to another page, how do I get the previous page if I want to check its URL? 
(4) have we implemented "feature policy" [3] yet? how to request this attribute?

Thanks!

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Frame.h?l=150
[2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/media/AutoplayPolicy.cpp?l=84
[3] https://wicg.github.io/feature-policy/
Flags: needinfo?(sshih)
Flags: needinfo?(bugs)
(1) Maybe document. Inner window changes if document.write is used after page load.
(though, document may change, and inner window stay the same when initial about:blank is replaced with a new document)


(2) Don't add an event listener for this. Perhaps just make EventStateManager to set a flag on nsIDocument when needed. EventStateManager dispatches click event.
(3) Oh, this is for "if navigating in the same eTLD+1, the new website would also have permission". Sounds like we need to propagate the flag when replacing inner windows during new page load.


How does this all work if there are nested iframes. Say, iframe A from domain 'a', and it has subframe B from domain 'b'.
Now there is another top level iframe from 'b' and one clicks that. Should B get the permission too?
A certainly shouldn't.
Flags: needinfo?(bugs)
We haven't implemented the feature policy yet.
(In reply to Olli Pettay [:smaug] from comment #2)
> (1) Maybe document. Inner window changes if document.write is used after
> page load.
> (though, document may change, and inner window stay the same when initial
> about:blank is replaced with a new document)

OK, I would add this flag on the document.

> (2) Don't add an event listener for this. Perhaps just make
> EventStateManager to set a flag on nsIDocument when needed.
> EventStateManager dispatches click event.

OK.

> (3) Oh, this is for "if navigating in the same eTLD+1, the new website would
> also have permission". Sounds like we need to propagate the flag when
> replacing inner windows during new page load.
> 
> 
> How does this all work if there are nested iframes. Say, iframe A from
> domain 'a', and it has subframe B from domain 'b'.
> Now there is another top level iframe from 'b' and one clicks that. Should B
> get the permission too?
> A certainly shouldn't.

I don't want to set the domains restriction on frame, since it's very common use case that embedder (main frame) has different domain with its embeded iframe. 

The basic and preliminary idea is if the top frame has permission, then all its child frames also have permission. Next steps for enhancement is, child frame could get the permission only when it has set the "feature-policy" flag and its parent frame has permission.

---

What I want to do in this bug is, to achieve the basic ability "know whether the website has been interacted by specific user gestures". That would be a criteria to help us decide whether website could be allowed the autoplay. If website has been activated by those user gestures, then it would get the autoplay permission.

Then, will open follow-up to do following enhancements,

1. Handle the cases navigating to same/different domain
For UX perspective, maybe we should keep the permission when navigating to the same domain. But we're still discussing the related UX spec, I would implement it after spec is more completed.

2. checking "feature-policy" to decide whether iframe should get the permission
Since we haven't implemented "feature-policy" yet, I would use a simple way that if the top frame has permission, then all its child frames also have permission. 

After implemented "feature-policy", then child frame could get the permission only when it has set the "feature-policy" flag and its parent frame has permission.

---

For the basic implementation, my idea is,
1. Add checking in EventStateManager
   - if the event is specific user gesture, then activate the document
2. When document is activating, we check all its parent, and activate them
3. In our autoplay policy, we check whether document is activated or not
   - if document is activated, then give it permission
   - if document is not activated, along its parent chain and check whether any of its parent has permission.
     if any of them has permission, then also give document permission.

How do you think my proposal?
Thanks!
Flags: needinfo?(bugs)
Blocks: 1382574
> 
> The basic and preliminary idea is if the top frame has permission, then all
> its child frames also have permission.
Really? All the ads in a page would get the permission. That doesn't sound too good.
Perhaps you mean eTLD+1 frames only?



> What I want to do in this bug is, to achieve the basic ability "know whether
> the website has been interacted by specific user gestures". That would be a
> criteria to help us decide whether website could be allowed the autoplay. If
> website has been activated by those user gestures, then it would get the
> autoplay permission.
ok, so you could add the flag on topmost eTLD+1 of the frame handling click. And then when
checking the flag, iterate through parent frames.


> For the basic implementation, my idea is,
> 1. Add checking in EventStateManager
>    - if the event is specific user gesture, then activate the document
> 2. When document is activating, we check all its parent, and activate them
> 3. In our autoplay policy, we check whether document is activated or not
>    - if document is activated, then give it permission
>    - if document is not activated, along its parent chain and check whether
> any of its parent has permission.
>      if any of them has permission, then also give document permission.
> 
> How do you think my proposal?
> Thanks!
Something like that yes, if eTLD+1 is handled reasonable. Giving permission to ads when user interacts with non-ad frame, doesn't sound good.


There can be hierarchies like, top level, from domain A -> ad domain B iframe -> subiframe from domain A.
When should B get the privilege?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #5).
> Really? All the ads in a page would get the permission. That doesn't sound
> too good.
> Perhaps you mean eTLD+1 frames only?

Just want to make sure I didn't misunderstand the term "eTLD+1".

We've maintained a list for all eTLDs [1], does the term "eTLD+1" mean the URL with one more name before eTLD?

eg. 
eTLD = "example.com"
eTLD+1 = "foo.example.com"

[1] https://publicsuffix.org/list/public_suffix_list.dat

> ok, so you could add the flag on topmost eTLD+1 of the frame handling click.
> And then when
> checking the flag, iterate through parent frames.

OK, so if the top frame has permission, its child eTLD+1 frame would also have permission. Is that right?

does this function is the right way to get the eTLD+1? [2]

[2 https://searchfox.org/mozilla-central/rev/30ead7d1ae5bf95b8bc0fd67b950cd46ca05e32c/netwerk/dns/nsIEffectiveTLDService.idl#79] 

> There can be hierarchies like, top level, from domain A -> ad domain B
> iframe -> subiframe from domain A.
> When should B get the privilege?

For me, I think domain B iframe could only get the permission when user has actually interacted with it.

When user interacts with top domain A frame, the permission would only be propagated to its children frame which is also on the domain A.

---

Another question is, we use this way [3] to go through all its parent.
Does this function always go through all parents along the parent chain? Would it be possible to stop at some point? eg. the parent iframe has "mozbrowser"

[3] https://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/dom/events/EventStateManager.cpp#568 

Thanks!
Flags: needinfo?(bugs)
After consulted with security team folks, they said that generally the permission would follow the same-origin policy, maybe we should check whether parent and child frame have same principle instead of eTLD?
What do other browsers do? But maybe we can start with same-origin.
Though, if we know others do eTLD+1, then same origin might not be well enough web compatible.

And if you want to handle mozbrowser, perhaps you could just iterate through docshell hierarcy, since there mozbrowser is explicitly handled https://searchfox.org/mozilla-central/rev/30ead7d1ae5bf95b8bc0fd67b950cd46ca05e32c/docshell/base/nsDocShell.cpp#3531,3536
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #8)
> What do other browsers do? But maybe we can start with same-origin.
> Though, if we know others do eTLD+1, then same origin might not be well
> enough web compatible.

Chrome won't care about the domain relationship of different frames, they would only use "policy flag" to decide whether need to give the permission.

Safari doesn't do thing like that, they use the setting to control which websites won't be blocked.

> And if you want to handle mozbrowser, perhaps you could just iterate through
> docshell hierarcy, since there mozbrowser is explicitly handled
> https://searchfox.org/mozilla-central/rev/
> 30ead7d1ae5bf95b8bc0fd67b950cd46ca05e32c/docshell/base/nsDocShell.cpp#3531,
> 3536

OK, I'll try that. Thanks.
Comment on attachment 8927723 [details]
Bug 1415444 - part1 : add flag to record whether the document has been activated by specific user gestures.

https://reviewboard.mozilla.org/r/198986/#review204026


C/C++ static analysis found 3 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: dom/base/nsDocument.cpp:13712
(Diff revision 1)
> +
> +  return mUserHasActivatedInteraction;
> +}
> +
> +nsIDocument*
> +nsIDocument::GetFirstParentDocumentWithSamePrincipal(nsCOMPtr<nsIPrincipal> aPrincipal)

Warning: The parameter 'aprincipal' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]

::: dom/base/nsDocument.cpp:13724
(Diff revision 1)
> +      return nullptr;
> +    }
> +
> +    if (isEqual) {
> +      return parent;
> +    } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

::: dom/base/nsDocument.cpp:13733
(Diff revision 1)
> +  MOZ_ASSERT(!parent);
> +  return nullptr;
> +}
> +
> +nsIDocument*
> +nsIDocument::GetSameTypeParentDocument(nsCOMPtr<nsIDocument> aDoc)

Warning: The parameter 'adoc' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]
Comment on attachment 8927723 [details]
Bug 1415444 - part1 : add flag to record whether the document has been activated by specific user gestures.

https://reviewboard.mozilla.org/r/198986/#review204134

::: dom/base/nsDocument.cpp:13701
(Diff revision 2)
> +nsIDocument::HasBeenUserActivated()
> +{
> +  if (!mUserHasActivatedInteraction) {
> +    // If one of its parent on the parent chain has been activated and has same
> +    // principal, then this child would also be treated as activated.
> +    nsCOMPtr<nsIDocument> parent =

This could be just nsIDocument* to be a tad faster-

::: dom/base/nsDocument.cpp:13715
(Diff revision 2)
> +
> +nsIDocument*
> +nsIDocument::GetFirstParentDocumentWithSamePrincipal(const nsCOMPtr<nsIPrincipal>& aPrincipal)
> +{
> +  MOZ_ASSERT(aPrincipal);
> +  nsCOMPtr<nsIDocument> parent = GetSameTypeParentDocument(this);

This could be nsIDocument* to be a tad faster

::: dom/base/nsDocument.cpp:13733
(Diff revision 2)
> +  MOZ_ASSERT(!parent);
> +  return nullptr;
> +}
> +
> +nsIDocument*
> +nsIDocument::GetSameTypeParentDocument(const nsCOMPtr<nsIDocument>& aDoc)

Why nsCOMPtr<nsIDocument> as param. That is unusual.
nsIDocument* should be fine.
Attachment #8927723 - Flags: review?(bugs) → review+
Comment on attachment 8927724 [details]
Bug 1415444 - part2 : add log.

https://reviewboard.mozilla.org/r/198988/#review204138

you really think this is useful even after initial implementation of this stuff? But fine r+
Attachment #8927724 - Flags: review?(bugs) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 45d21d8e4249 -d b6daeeac264c: rebasing 433758:45d21d8e4249 "Bug 1415444 - part1 : add flag to record whether the document has been activated by specific user gestures. r=smaug"
merging dom/base/nsDocument.cpp
merging dom/base/nsIDocument.h
merging dom/events/EventStateManager.cpp
merging dom/events/EventStateManager.h
warning: conflicts while merging dom/base/nsIDocument.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f318e1ed5c37
part1 : add flag to record whether the document has been activated by specific user gestures. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d61d99ecc3c8
part2 : add log. r=smaug
https://hg.mozilla.org/mozilla-central/rev/f318e1ed5c37
https://hg.mozilla.org/mozilla-central/rev/d61d99ecc3c8
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: