1503724 - Initial custom tab load no longer displays progress bar

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Custom Tabs, defect)

Description

[Jan Henning [:JanH]]

The initial load that creates the custom tab no longer displays a progress bar. When reloading the page afterwards or navigating to some other page within the same custom tab the progress bar works again.

mozregression says https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=971943621f6e52daa0e94e48c3d846567978ff26&tochange=6bb8792d978ca0b0b84a0fd1218260392b897ed1

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

I can reproduce this, but probably not going to have any time to work on it soon due to travel. Dylan, can you roll this in with the other GV/Fennec fixes you're doing?

Comment 2

[Dylan Roeh (:droeh) (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #1)
> I can reproduce this, but probably not going to have any time to work on it
> soon due to travel. Dylan, can you roll this in with the other GV/Fennec
> fixes you're doing?

Yup, I'll take a look.

Comment 3

[Dylan Roeh (:droeh) (he/him)]

Attachment: Fix custom tabs progress bar for first loads

The problem we were running into here was that PAGE_START was being sent before we enabled GeckoViewProgressChild, so we weren't yet listening for it to know when to start tracking page load progress. With this patch we listen for just NOTIFY_STATE_NETWORK events beginning in onInit() and then expand that to other necessary progress events in onEnable().

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 9023029 [details] [diff] [review]
Fix custom tabs progress bar for first loads

Review of attachment 9023029 [details] [diff] [review]:
-----------------------------------------------------------------

I see one potential problem, but I don't really know the progress tracker code well enough to comment on most of the implications. Kicking over to esawin.

::: mobile/android/chrome/geckoview/GeckoViewProgressChild.js
@@ +56,5 @@
>        this.progressFilter.removeProgressListener(this);
>        let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
>                          .getInterface(Ci.nsIWebProgress);
>        webProgress.removeProgressListener(this.progressFilter);
> +      this.progressFilter = null;

It seems like we actually want to do the same thing as onInit(), no?

Comment 5

[Eugen Sawin [:esawin]]

Comment on attachment 9023029 [details] [diff] [review]
Fix custom tabs progress bar for first loads

Review of attachment 9023029 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't necessarily limited to missing PAGE_START, is it?
We could potentially miss other events which are not covered by NOTIFY_STATE_NETWORK, at which point the progress tracker state would be out of sync. The consequences for missing progress events range from wrong load progress values to a stuck progress bars.

I fear that to solve this correctly, we have to track all events on initialization.
We also have to make sure we handle detaching and re-attaching of the user delegate correctly.

r- because I think missing other progress events could have negative side effects.

::: mobile/android/chrome/geckoview/GeckoViewProgressChild.js
@@ +15,5 @@
>      debug `onInit`;
> +
> +    ProgressTracker.onInit(this);
> +
> +    let flags = Ci.nsIWebProgress.NOTIFY_STATE_NETWORK;

const

@@ +20,5 @@
> +    this.progressFilter =
> +      Cc["@mozilla.org/appshell/component/browser-status-filter;1"]
> +      .createInstance(Ci.nsIWebProgress);
> +    this.progressFilter.addProgressListener(this, flags);
> +    let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)

const

@@ +30,5 @@
>      debug `onEnable`;
>  
> +    if (this.progressFilter) {
> +      this.progressFilter.removeProgressListener(this);
> +      let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)

const

Comment 6

[Dylan Roeh (:droeh) (he/him)]

Attachment: Fix custom tabs progress bar for first loads

Yeah, that's a good point -- I think the best approach at the moment is just to bite the bullet and enable the listeners always. If we stop listening at any point, we run the risk of either not having progress or having bogus progress for a load.

Comment 7

[Eugen Sawin [:esawin]]

Comment on attachment 9023977 [details] [diff] [review]
Fix custom tabs progress bar for first loads

Review of attachment 9023977 [details] [diff] [review]:
-----------------------------------------------------------------

Listening to events without a delegate attached isn't ideal, but I think this would be the most reliable fix for this.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bf1c95a5db3
Fix progress bar on first load in Fennec custom tabs. r=esawin

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/4bf1c95a5db3

Comment 10

[Stefan Deiac]

Verified as fixed in the latest version of Nightly 65.0a1 (2018-11-12) with Nokia 6(Android 7.1.1) and OnePlus Two(Android 6.0.1).

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 12

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 9023977 [details] [diff] [review]
Fix custom tabs progress bar for first loads

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1437988

User impact if declined: A race might result in some pages not showing a progress bar in custom tabs and other GV apps, particularly a first load.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Overall behavior is the same, we just start listening for progress events sooner.

String changes made/needed: N/A

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 9023977 [details] [diff] [review]
Fix custom tabs progress bar for first loads

fix for fennec custom tabs, approved for 64.0b11

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0ab056dc26a8