1748927 - GeckoSessionHandler.getDelegate can be called from any thread but is not thread safe

Status: NEW

(GeckoView :: General, defect, P2)

Description

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

GeckoSession.load can be called from @AnyThread but it calls mNavigationHandler.getDelegate, i.e. GeckoSessionHandler.getDelegate, which does not appear to be thread safe. GeckoSessionHandler does not synchronize access to mDelegate, which is returned from getDelegate and set in setDelegate.

In Java, if a mutable variable is accessed from multiple threads, you must synchronize access to it if you don't want stale values because a write from one thread is not guaranteed to be visible to another thread unless a synchronization event occurred (e.g. synchronized or volatile).

Since setNavigationDelegate (which calls setDelegate) must be called on the UiThread and mNavigationHandler.getDelegate() may be called from AnyThread (via GeckoSession.load), GeckoSessionHandler.mDelegate runs into the staleness issue mentioned above and should be made thread safe.

Comment 1

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

Bug 1737824 allowed GeckoSession.load to be called off the main thread so this may be a regression from it. However, some of setDelegate's callers are annotated @AnyThread (example) so I think this concurrency issue existed before this.

Comment 2

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

I tried to fix this but it got too complex for me given my limited understanding of the code. The challenge is that GeckoSessionHandler leaked its thread safety needs to other classes:

• if GeckoSessionHandler.setDelegate or GeckoSessionHandler.getDelegate() is ever called from a background thread for a given handler:
  • the delegate must be internally thread safe (since the delegate is guaranteed to be used on the ui thread by handleMessage)
  • the usage of the delegate is not mutually exclusive so it must be okay with being interwoven, e.g. UI thread: getDelegate(); BG thread: setDelegate(newDelegate); UI thread: continues to use old delegate, or synchronized
• GeckoSessionHandler registers itself as a listener to GeckoSession.getEventDispatcher(). Without an understanding of the code, it's theoretically possible for handleMessage to be called at any time other methods are being called on a background thread and I'm not sure if that's safe

I'll push my WIP.

Comment 3

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

Attachment: WIP: Bug 1748927 - set applicable GeckoSession fields to final.

The IDE suggested I can make this change. This is change is unrelated to the
listed bug and is just general cleanup though setting fields to final
strengthens concurrency guarantees.

Comment 4

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

Attachment: WIP: Bug 1748927 - add GeckoSessionHandlerTest.

These tests help ensure the variables we're going to change are still working
correctly after the change.

Depends on D135432

Comment 5

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

Attachment: WIP: Bug 1748927 - make GeckoSessionHandler thread safe WIP.

Depends on D135433

Comment 6

[Chris Peterson [:cpeterson]]

Mike, do we still need to these thread safety fixes if we implement Declarative onLoadRequest bug 1734923? The soonest we might fix bug 1734923 is Q3 or Q4. Should we land your thread safety fixes in the meantime?

Comment 7

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

(In reply to Chris Peterson [:cpeterson] from comment #6)

Mike, do we still need to these thread safety fixes if we implement Declarative onLoadRequest bug 1734923?

Yes – this code is unrelated. It's used for callbacks to the application, for example HistoryDelegate.onHistoryStateChange. fwiw, it's hard to predict concurrency issues but I suspect the way this would manifest is that 1) the application would miss some callbacks or receive the callbacks on an object that they thought was no longer used and 2) state used by the classes inside the callbacks may similarly use out-of-date values.

Should we land your thread safety fixes in the meantime?

For this bug, my WIP is not ready to land – I was unable to solve the issue (see comment 2 for details).

Comment 8

[Chris Peterson [:cpeterson]]

Back to P2 for consideration in 2022 H2