1461738 - Don't unregister listeners in GeckoSessionHandler

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P3)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Unregistering listeners in GeckoSessionHandler can lead to race conditions when clearing delegates.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1461738 - 1. Remove always-listen option in GeckoSessionHandler;

The "always-listen" option for GeckoSessionHandler was something we used
for prompts and permissions, but we don't need it anymore.

Review commit: https://reviewboard.mozilla.org/r/244056/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244056/

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1461738 - 2. Don't unregister listeners in GeckoSessionHandler;

Unregistering listeners when we clear a delegate can lead to some event
races between the Gecko and UI thread. It's unclear to me if
unregistering events actually has much benefit, so I think we should
just not unregister events at all.

Review commit: https://reviewboard.mozilla.org/r/244058/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244058/

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1461738 - 3. Add test for clearing navigation delegate on load;

Add a test where we clear the navigation delegate right after starting a
load. This test exposes the race between Gecko thread and UI thread when
clearing a delegate.

Review commit: https://reviewboard.mozilla.org/r/244060/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244060/

Comment 4

[Eugen Sawin [:esawin]]

Comment on attachment 8975873 [details]
Bug 1461738 - 1. Remove always-listen option in GeckoSessionHandler;

https://reviewboard.mozilla.org/r/244056/#review250014

Comment 5

[Eugen Sawin [:esawin]]

Comment on attachment 8975874 [details]
Bug 1461738 - 2. Don't unregister listeners in GeckoSessionHandler;

https://reviewboard.mozilla.org/r/244058/#review250020

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8975875 [details]
Bug 1461738 - 3. Add test for clearing navigation delegate on load;

https://reviewboard.mozilla.org/r/244060/#review250022

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf893cb5f091
1. Remove always-listen option in GeckoSessionHandler; r=esawin
https://hg.mozilla.org/integration/autoland/rev/ee2a5ca67ea6
2. Don't unregister listeners in GeckoSessionHandler; r=esawin
https://hg.mozilla.org/integration/autoland/rev/5009e8a9734c
3. Add test for clearing navigation delegate on load; r=esawin

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/bf893cb5f091
https://hg.mozilla.org/mozilla-central/rev/ee2a5ca67ea6
https://hg.mozilla.org/mozilla-central/rev/5009e8a9734c