1174966 - Ensure pointer lock event is dispatched

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+11]

There are two bugs which may cause we do not dispatch any event for a request call:
1. we use a problematic mechanism to track async fullscreen call;
2. we do not check pending pointer lock request if no approval is needed for fullscreen.

I suppose these bugs, especially the first one, cause intermittent failures like bug 788164 or bug 931445.

The first issue in more details:
We use a flag mAsyncFullscreenPending on document. The nsCallRequestFullScreen backups that flag when constructed, and restores it when it executes. This is broken because nsCallRequestFullScreen is called in FIFO order.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 1 - reduce space for recording cancelled pointer lock requests

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - replace pending fullscreen flag with a counter

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 3 - use single FullscreenRequest object in lifetime of a request

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 4 - move counter manipulation to the request object

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 5 - redispatch pointer lock request when fullscreen requests finish

Comment 6

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

Comment on attachment 8623484 [details] [diff] [review]
patch 1 - reduce space for recording cancelled pointer lock requests

I don't know why to use 'auto' here. It doesn't improve readability in this case.

Comment 7

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

Comment on attachment 8623485 [details] [diff] [review]
patch 2 - replace pending fullscreen flag with a counter

Same comment about 'auto' usage, especially given that the existing code uses
nsDocument* doc = static_cast...

I think 'if (doc->mPendingFullscreenRequests'
would be easier to read if it was 
if (doc->mPendingFullscreenRequests > 0.

But what happens if mPendingFullscreenRequests overflows? uint8_t isn't that big.

Comment 8

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

Comment on attachment 8623486 [details] [diff] [review]
patch 3 - use single FullscreenRequest object in lifetime of a request


>-  FullScreenOptions opts;
>-  opts.mIsCallerChrome = nsContentUtils::IsCallerChrome();
>+  auto request = MakeUnique<FullscreenRequest>(this);
Definitely not auto here. It totally hides what kind of thing request is.
Same also elsewhere in this patch

Comment 9

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

Comment on attachment 8623487 [details] [diff] [review]
patch 4 - move counter manipulation to the request object

Tiny bit weird to have mPendingFullscreenRequests bound to the number of
FullscreenRequest objects alive, not number of FullscreenRequest objects which haven't been handled yet. But I guess this should be fine.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8623484 [details] [diff] [review]
> patch 1 - reduce space for recording cancelled pointer lock requests
> 
> I don't know why to use 'auto' here. It doesn't improve readability in this
> case.

It doesn't improve readability but it doesn't hurt that either.

I think it has been commonly accepted that it is generally a good idea to use "auto" for value returned from things like static_cast, since that help avoiding redundancy, and make code more concise.

In your "Use of 'auto'" post in dev-platform mailing list [1], there are several people mentioned and agreed with this pattern: Kyle Huey, Martin Thomson, Daniel Holbert, Aaron Klotz, Joshua Cranmer. This is probably the most accepted usecase for 'auto'.

[1] https://groups.google.com/forum/#!topic/mozilla.dev.platform/tcESI7M5hEY

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8623485 [details] [diff] [review]
> patch 2 - replace pending fullscreen flag with a counter
> 
> I think 'if (doc->mPendingFullscreenRequests'
> would be easier to read if it was 
> if (doc->mPendingFullscreenRequests > 0.

OK.

> But what happens if mPendingFullscreenRequests overflows? uint8_t isn't that
> big.

Right, uint8_t isn't that big, but I'm not very concerned about its being overflow. The number of pending fullscreen request doesn't affect whether a request would be approved. It only affects when the pointer lock request will be measured again.

Generally, authors should not request fullscreen so many times (>200) in a short time (at most ~1s), since that won't make anything better. But if someone does that and makes the counter overflow, we would just show a popup for the permission during entering fullscreen, which slightly hurt user experience.

Since behavior of overflow/underflow of unsigned integer is well-defined, pending pointer lock request is always guaranteed to be replied when all fullscreen requests finish, anyway.

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8623486 [details] [diff] [review]
> patch 3 - use single FullscreenRequest object in lifetime of a request
> 
> >-  FullScreenOptions opts;
> >-  opts.mIsCallerChrome = nsContentUtils::IsCallerChrome();
> >+  auto request = MakeUnique<FullscreenRequest>(this);
> Definitely not auto here. It totally hides what kind of thing request is.
> Same also elsewhere in this patch

Actually, the function MakeUnique is designed to be used with 'auto'. Otherwise, this line would be
> UniquePtr<FullscreenRequest> request(new FullscreenRequest(this));
which is meaninglessly redundant.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 8623487 [details] [diff] [review]
> patch 4 - move counter manipulation to the request object
> 
> Tiny bit weird to have mPendingFullscreenRequests bound to the number of
> FullscreenRequest objects alive, not number of FullscreenRequest objects
> which haven't been handled yet. But I guess this should be fine.

FullscreenRequest object is released when it is dropped or it is handled. It may live several lines longer after it is handled, but I think it should be fine, as that doesn't change any order of events.

Comment 14

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

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #10)
> 
> It doesn't improve readability but it doesn't hurt that either.
I'd say it does hurt readability. When I'm reading code somewhere down below, and try to remember the type of the
variable, auto forces me to read also the part after '='. Without auto just reading the variable declaration is enough.

Comment 15

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

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #12)
> Actually, the function MakeUnique is designed to be used with 'auto'.
> Otherwise, this line would be
> > UniquePtr<FullscreenRequest> request(new FullscreenRequest(this));
> which is meaninglessly redundant.

No. It at least tells what is the type of 'request'. That is what I as a code reviewer need to know. 
auto foo = MakeUnique<...> forces one to check what MakeUnique does.
UniquePtr.h has even a comment about the issue
http://mxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h?rev=cc8e3db0622b&mark=618-621#618

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] from comment #15)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #12)
> > Actually, the function MakeUnique is designed to be used with 'auto'.
> > Otherwise, this line would be
> > > UniquePtr<FullscreenRequest> request(new FullscreenRequest(this));
> > which is meaninglessly redundant.
> 
> No. It at least tells what is the type of 'request'. That is what I as a
> code reviewer need to know. 
> auto foo = MakeUnique<...> forces one to check what MakeUnique does.
> UniquePtr.h has even a comment about the issue
> http://mxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.
> h?rev=cc8e3db0622b&mark=618-621#618

So now you know that. It's a good news, isn't it :)

Searching our codebase shows that almost all MakeUnique()s are used with auto if assigning to a local variable.

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 2 - replace pending fullscreen flag with a counter

Comment 18

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

Comment on attachment 8624141 [details] [diff] [review]
patch 2 - replace pending fullscreen flag with a counter

So what happens if mPendingFullscreenRequests overflows? I'm just thinking about some error in a web page script, some unexpected code path which ends up doing many fullscreen requests.
doc->mPendingFullscreenRequests > 0 may end up being false, when it shouldn't be.

Perhaps change
if (!aElement) {
  return;
}
to  
if (!aElement || mPendingFullscreenRequests == UINT8_MAX) {
  return;
}
in nsDocument::AsyncRequestFullScreen. I think that would lead to saner behavior in the edge case.

With that, r+

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Since the manipulation of the request counter is moved to the constructor of FullscreenRequestin patch 4, it would probably better to check that before creating any request object.

I'd like to keep patch 2 as-is, and add a new patch for adding that sanity check. Does that sound good to you?

Comment 20

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

Sounds good.

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Errrr... Found it harder than thought.

The problem for sanity check is, on e10s, if the parent process receives too many requests, and abandons because of sanity check, the child process might never receive the reply, because there is no corresponding pending request in the parent side.

If we want to fix this, we would need a mechanism for the parent to notify the child to clear the pending request. I think this might be an overkill just for a sanity check.

As I described in comment 11, I don't think it would really cause any security problem, but instead, only a slight punishment on user experience.

Given this, would you mind me landing the current patchset as-is?

(If we really want to avoid any issue on this, I guess a more realistic way would be to limit the number of fullscreen requests per user input.)

Comment 22

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

What does the UI look if we overflow the counter? I mean, does it effectively break something, or is it just annoying?



But I guess I could live with the patch as is, though, I wouldn't mind using uint32_t for the counter.
Either way, uint8_t or uint32_t.

Comment 23

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] from comment #22)
> What does the UI look if we overflow the counter? I mean, does it
> effectively break something, or is it just annoying?

Just annoying. But I made a mistake. When the counter overflows, a request may be rejected even though it would otherwise be accepted.

So if a pointer lock request arrives at the point there are exactly 256x pending fullscreen requests (so the counter is 0), the "pending fullscreen request" check passes. Then, it has three ends:
1. If we have been in fullscreen, the request is accepted as it should, but then the pointer will be immediately unlocked because of the following fullscreen change.
2. The request is rejected because it was not called from a user input. When the counter doesn't overflow, we won't reject it because we are going to fullscreen and will accept it then.
3. The request causes a popup ask user for permission when we are entering fullscreen.

So the result is pointer lock may not work as expected if it is really lucky. But once all fullscreen requests finishes, new pointer lock request would work well.

> But I guess I could live with the patch as is, though, I wouldn't mind using
> uint32_t for the counter.
> Either way, uint8_t or uint32_t.

I don't think it worth 4 bytes for that. Actually I even think it is enough to give it only 4bit :)

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef8a53704b9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/54f1728e0861
https://hg.mozilla.org/integration/mozilla-inbound/rev/49fd389060e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/83e032875162
https://hg.mozilla.org/integration/mozilla-inbound/rev/88c6b4df3eb2

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef8a53704b9f
https://hg.mozilla.org/mozilla-central/rev/54f1728e0861
https://hg.mozilla.org/mozilla-central/rev/49fd389060e2
https://hg.mozilla.org/mozilla-central/rev/83e032875162
https://hg.mozilla.org/mozilla-central/rev/88c6b4df3eb2