1646493 - Fix GetInProcessScriptableTop usage in pointer lock code for OOP iframes

Status: RESOLVED FIXED

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

Description

[Kris Maglione [:kmag]]

It's possible we have some other code that makes things work when OOP frames are involved, but I'm not sure.

Comment 1

[Kris Maglione [:kmag]]

(This also includes nsContentUtils::IsInPointerLockContext I suppose)

Comment 2

[Chris Peterson [:cpeterson]]

Tracking "Figure out GetInProcessTop usage" bugs for Fission M6b.

Comment 3

[Neha Kochar [:neha]]

Olli, can you do this audit and point it to someone else in DOM to fix?

Comment 4

[Edgar Chen [:edgar]]

I will take a look at this.

Comment 5

[Edgar Chen [:edgar]]

There are two GetInProcessScriptableTop usages for pointer lock, both of them doesn't work when OOP frames are involved,

1. https://searchfox.org/mozilla-central/rev/73a14f1b367948faa571ed2fe5d7eb29460787c1/dom/base/Document.cpp#14303 is to check if toplevel document is active, this now doesn't work as expected on fission.

   • Other than that, https://searchfox.org/mozilla-central/rev/73a14f1b367948faa571ed2fe5d7eb29460787c1/dom/base/Document.cpp#14275 is also incorrect, it could possible that pointer lock is requested by an OOP iframe, checking EventStateManager::sPointerLockedElement won't work.

2. https://searchfox.org/mozilla-central/rev/73a14f1b367948faa571ed2fe5d7eb29460787c1/dom/base/nsContentUtils.cpp#6620-6621 is to check if pointer lock is requested under certain toplevel document, EventStateManager::sPointerLockedDoc won't point to the right thing if the lock is requested by an OOP iframe.

Comment 6

[Edgar Chen [:edgar]]

Attachment: Bug 1646493 - Make GetPointerLockError Fission compatible;

by replacing the usage of GetInProcessScriptableTop with {Browsing/Window}Context

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ff509769a96
Make GetPointerLockError Fission compatible; r=smaug

Comment 8

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/0ff509769a96

Comment 9

[Edgar Chen [:edgar]]

Attachment: Bug 1646493 - Make pointer auto-unlocking for active top-level document changes fission compatible;

Unfortunately, this change makes content process and chrome process doesn't go
through the same code path because nsFocusManager handles their state
separately on purpose.

Comment 10

[Edgar Chen [:edgar]]

After working on bug 1662587 and understand focus handling a bit more, I think we don't need to wait for bug 1662587, could fix this bug separately.

Comment 11

[Edgar Chen [:edgar]]

Attachment: Bug 1646493 - Tests;

Comment 12

[Phabricator Automation]

Comment on attachment 9175877 [details]
Bug 1646493 - Tests;

Revision D90313 was moved to bug 1662587. Setting attachment 9175877 [details] to obsolete.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90e073a9c89d
Make pointer auto-unlocking for active top-level document changes fission compatible; r=hsivonen

Comment 14

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/90e073a9c89d