1748945 - `GeckoSession.mWindow` is not thread safe but is annotated that it can be accessed from multiple threads

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

GeckoSession.mWindow does not have synchronized access. GeckoSession.isOpen is annotated that it can be called from @AnyThread so GeckoSession.mWindow will need to be synchronized or otherwise changed to be thread safe.

The variable is initialized on the UI thread in GeckoSession.open and nulled again in GeckoSession.close or abandonWindow so the consequence of not fixing this is that background threads may read a stale value – either the initial null value or they'll get the Window after it's been closed.

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

I think the @AnyThread in there is a mistake, I don't think we can reliably tell a consumer if a session is open on a background thread. Even with synchronized access to mWindow code like this will always be broken on a background thread:

        if (!session.isOpen()) {
          session.open(sGeckoRuntime);
        }

because another thread could open the session between the isOpen() and the session.open statements.

We should just change isOpen to @UiThread and verify that all calls actually happen on the UI thread.

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1748945 - Change isOpen to @UiThread.

The fact that isOpen is @AnyThread seems to be a mistake, I don't think we can
reliably tell a consumer if a session is open on a background thread.

Even with synchronized access to mWindow code like this would always be broken
on a background thread:

if (!session.isOpen()) {
  session.open(sGeckoRuntime);
}

because another thread could open the session between the isOpen() and the
session.open statements.

This patch changes isOpen to @UiThread. I verified that all calls actually
happen on the UI thread, except for the calls in GeckoSessionSettings where we
are trying to handle the case where a session was closed and then we would
allow modifying read-only settings. I think in that case we should ask the
embedder to use a new SessionSettings instance instead of forcing all calls to
be @UiThread.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f24e597defe1
Change isOpen to @UiThread. r=calu

Comment 4

[Atila Butkovits]

Backed out for causing Lint failure.

Backout link: https://hg.mozilla.org/integration/autoland/rev/93daaede80eef1add213d205222cea9b39ae735f

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=f24e597defe1e9c072b3c0a49333cc7ae57f5170&selectedTaskRun=Q_xcb6tWT9C__vRodVEkSQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=370338043&repo=autoland&lineNumber=34871

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2149ff735175
Change isOpen to @UiThread. r=calu

Comment 6

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2149ff735175