1621480 - GeckoSession.HistoryDelegate.onHistoryStateChange is not fired unless GeckoSession.ProgressDelegate.onSessionStateChange is set

Status: RESOLVED FIXED

(GeckoView :: General, defect, P2)

Description

[staleydyla.n]

Attachment: MainActivity.kt

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.132 Safari/537.36

Steps to reproduce:

• Create a GeckoSession without attaching a ProgressDelegate
• Load a webpage with links to another page
• Navigate to another page

Actual results:

onHistoryStateChange handler is not fired on navigation

Expected results:

onHistoryStateChange should be fired, logging "currentIndex: 1"

Comment 1

[staleydyla.n]

Attaching the ProgressDelegate (by uncommenting line 33) and performing navigation results in the onHistoryStateChange handler being correctly fired

Comment 2

[[:owlish] 🦉 PST]

On the subsequent navigations, is onHistoryStateChange fired? Is currentIndex incremented or does it stay 0 all the time?

Comment 3

[staleydyla.n]

(In reply to [:owlish] from comment #2)

On the subsequent navigations, is onHistoryStateChange fired? Is currentIndex incremented or does it stay 0 all the time?

onHistoryStateChange is never fired. I don't think I can check currentIndex since that's a property of the HistoryList, and since the callback is never fired I don't have access to that.

Comment 4

[[:owlish] 🦉 PST]

I have some more questions. What happens when you do have progressDelegate? (like, when that line is uncommented) Does onHistoryStateChange behave correctly?
Also, can you tell us a bit more about your use case? It seems important for you not to have progressDelegate in there

Comment 5

[staleydyla.n]

(In reply to [:owlish] from comment #4)

I have some more questions. What happens when you do have progressDelegate? (like, when that line is uncommented) Does onHistoryStateChange behave correctly?
Also, can you tell us a bit more about your use case? It seems important for you not to have progressDelegate in there

Yes, when the progressDelegate is set (so the line is uncommented), the onHistoryStateChange callback is fired correctly on all navigation events.

As far as use case goes, I don't mind having a progressDelegate set, but based on comments on Matrix it sounds like it shouldn't be necessary. https://matrix.to/#/!kgzqJPKsBZFakDVGxV:mozilla.org/$DcCMHjyZvEwYkmshs838X0CMGz0mXKMeMn6BsU7PPic?via=mozilla.org&via=matrix.org&via=matrix.rmed.dev

Originally, I was using onHistoryStateChange to check if the back-stack was empty, but I've switched to using onCanGoBack instead. I wanted to file this issue though since it sounded like onHistoryStateChange should work without a progressDelegate.

Comment 6

[[:owlish] 🦉 PST]

Attachment: Bug 1621480 - Separate progress delegate and history delegate while avoiding race conditions

Move the history state logic to history delegate handler to separate progress delegate and history delegate

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd5ac7f5ef15
Separate progress delegate and history delegate while avoiding race conditions r=geckoview-reviewers,snorp

Comment 8

[[:owlish] 🦉 PST]

Hey, the patch has landed - can you please confirm it's working for you now?

Edit: I realized immediately it probably wasn't released yet. I guess we have to wait for the release and then confirm the patch works

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/cd5ac7f5ef15