Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Firefox Continually Refreshing Pages #202

Closed
KZeni opened this issue Jul 30, 2019 · 12 comments · Fixed by #279
Closed

Firefox Continually Refreshing Pages #202

KZeni opened this issue Jul 30, 2019 · 12 comments · Fixed by #279
Milestone

Comments

@KZeni
Copy link
Contributor

KZeni commented Jul 30, 2019

I've noticed that Firefox is continually refreshing on various pages of my site (after the "Allow Notifications" prompt is shown, but it refreshes before the user can really act on that prompt). I'm thinking it's tied to the

let refreshedPage = false;
navigator.serviceWorker.addEventListener( 'controllerchange', () => {
	if ( ! refreshedPage ) {
		refreshedPage = true;
		window.location.reload();
	}
} );

snippet added to the page by PWA.

Oddly, the homepage is pretty much the same as the other pages having the refresh issue (also having this same code snippet), but it's happily not refreshing again & again with the "Allow Notifications" prompt to be acted on.

Any idea of what might be the fix for this?

@KZeni KZeni changed the title Firefox continually refreshing page Firefox Continually Refreshing Pages Jul 30, 2019
@westonruter
Copy link
Collaborator

westonruter commented Jul 30, 2019

Any chance that the issue is fixed by #187 (for #183)? If you have enqueued scripts/styles that have time() as the version then this was causing endless reloads.

Otherwise, do you have a public URL to inspect?

@KZeni
Copy link
Contributor Author

KZeni commented Jul 30, 2019

Okay, I've implemented the change from the PR you mentioned, and it appears to have fixed the problem!

I've follow-up if I come across anything else, but I'm marking this as resolved per the patch (which I imagine will be included in the next official update.)

Thanks, @westonruter!

@KZeni KZeni closed this as completed Jul 30, 2019
@westonruter westonruter added the duplicate This issue or pull request already exists label Jul 30, 2019
@KZeni KZeni reopened this Jul 31, 2019
@KZeni
Copy link
Contributor Author

KZeni commented Jul 31, 2019

Hold on, I might've spoken too soon, @westonruter.

https://www.britespanbuildings.com/careers/ (see the raw HTML here: https://cloudup.com/coWgSij610s [if I implement a fix in the meantime]) is currently still experiencing the redirect/refresh loop in Firefox even after swapping out

$enqueued_scripts = array();
$revision .= ';deps=' . md5( wp_json_encode( array( wp_scripts()->queue, wp_styles()->queue ) ) );
foreach ( wp_scripts()->queue as $handle ) {		
	if ( isset( wp_scripts()->registered[ $handle ] ) ) {		
		$enqueued_scripts[ $handle ] = wp_scripts()->registered[ $handle ];		
	}		
}		
$enqueued_styles = array();		
foreach ( wp_styles()->queue as $handle ) {		
	if ( isset( wp_styles()->registered[ $handle ] ) ) {		
		$enqueued_styles[ $handle ] = wp_styles()->registered[ $handle ];		
	}		
}		
$revision .= ';deps=' . md5( wp_json_encode( compact( 'enqueued_scripts', 'enqueued_styles' ) ) );

for

$revision .= ';deps=' . md5( wp_json_encode( array( wp_scripts()->queue, wp_styles()->queue ) ) );

in wp-includes/components/class-wp-service-worker-navigation-routing-component.php per the proposed PR mentioned earlier.

@KZeni
Copy link
Contributor Author

KZeni commented Jul 31, 2019

I have since implemented add_filter( 'wp_service_worker_skip_waiting', '__return_false' ); as part of my theme's functions.php file, but I then also needed to change

let refreshedPage = false;
navigator.serviceWorker.addEventListener( 'controllerchange', () => {
	if ( ! refreshedPage ) {
		refreshedPage = true;
		window.location.reload();
	}
} );

to

<?php if ( wp_service_worker_skip_waiting() ) : ?>
let refreshedPage = false;
navigator.serviceWorker.addEventListener( 'controllerchange', () => {
	if ( ! refreshedPage ) {
		refreshedPage = true;
		window.location.reload();
	}
} );
<?php endif; ?>

in wp-includes/service-workers.php to make it so it doesn't leave Firefox in a refresh loop (it really seems like the presence of that one JS code block is causing the Firefox infinite refresh loop here).

Does this last adjustment make sense to implement in the plugin officially? It seems to resolve an issue when wp_service_worker_skip_waiting is set to false while having it act properly when wp_service_worker_skip_waiting is left as true.

Otherwise, is there something else causing this refresh loop and/or is there a better fix for this (ex. keeping that JS code snippet as always included, but find a way to have it be triggered more reasonably [again, my site was otherwise caught in a refresh loop, only in Firefox, with this one JS code snippet present as-is], add a different filter that's checked to determine if this code snippet is included or not [if wp_service_worker_skip_waiting isn't totally appropriate to be used here], etc.)

@KZeni
Copy link
Contributor Author

KZeni commented Jul 31, 2019

Another possible solution would be to update that same JS code block to be

let refreshedPage = false;
navigator.serviceWorker.addEventListener( 'controllerchange', () => {
	<?php if ( ! wp_service_worker_skip_waiting() ) : ?>
	const notification = document.getElementById( 'wp-admin-bar-pwa-sw-update-notice' );
	if ( notification ) {
		notification.style.display = 'block';
	}
	<?php else: ?>
	if ( ! refreshedPage ) {
		refreshedPage = true;
		window.location.reload();
	}
	<?php endif; ?>
} );

as this also resolves the Firefox refresh loop when wp_service_worker_skip_waiting is set too false.

@KZeni
Copy link
Contributor Author

KZeni commented Jul 31, 2019

I'll wait on proposing a pull request until I know which solution is best, but it seems like we might have some reasonable options to choose from.

@westonruter westonruter removed the duplicate This issue or pull request already exists label Jul 31, 2019
@westonruter
Copy link
Collaborator

I'm confused as to why Firefox is continually sending a controllerchange event here even when wp_service_worker_skip_waiting is disabled. More fundamentally, why is controllerchange continually being triggered even when wp_service_worker_skip_waiting is enabled?

The normal behavior currently is for an update to the service worker to cause the updated service worker to be installed and then for the page to reload. Once the page reloads, the service worker is checked for changes and if it is the same, then no update should be happening and thus no reload should occur.

I think the problem is that you may have two separate service workers being installed, and they are competing. In Firefox I can see the registered service worker is:

https://www.britespanbuildings.com/?firebase-messaging-sw

Can you try undoing the changes you made above, and then deactivate the plugin that adds this code:

<!-- Start Subscriber Embed Code -->
<script type="text/javascript">
var subscribersSiteId = '653953b7-7caa-4f2b-a839-184ad61fc132';
var subscribersServiceWorkerPath = '/?firebase-messaging-sw';
</script>
<script type="text/javascript" src="https://cdn.subscribers.com/assets/subscribers.js"></script>
<!-- End Subscriber Embed Code -->

What plugin is responsible for that?

@KZeni
Copy link
Contributor Author

KZeni commented Jul 31, 2019

This is from the https://subscribers.com plugin (https://wordpress.org/plugins/subscribers-com/) the client has installed & is using.

I have removed my modification, noticed the refresh kept happening, disabled the Subscribers.com plugin, and then I saw the issue was no longer happening. It does appear to be tied to the Subscribers.com plugin at this point.

Since the Subscribers.com plugin is a client request, I've re-enabled that plugin and re-implemented my modifications for the time being. As an aside, I've also posted about this at https://wordpress.org/support/topic/conflict-with-pwa-plugin-and-firefox/

@KZeni
Copy link
Contributor Author

KZeni commented Jul 31, 2019

Of course, I'll defer to you on what should be done as you're more familiar with PWA than I am.

@westonruter
Copy link
Collaborator

OK, I took a deeper look and unfortunately the two are not going to be compatible at this time.

For instance, taking a look at https://cdn.subscribers.com/assets/subscribers.js

Unminified, this file has the following code:

window.subscribersApp.requestFirebasePermission = function ( e ) {
    return new Promise( function ( s, i ) {
        window.subscribersApp.loadFirebase( e ).then( function ( e ) {
            firebase.messaging.isSupported() || i( "Not supported" ), e.onTokenRefresh( function () {
                e.getToken().then( function ( s ) {
                    e.registrationToUse.pushManager.getSubscription().then( function ( e ) {
                        window.subscribersApp.postToken( {
                            service: window.subscribersApp.service,
                            vendor: window.subscribersApp.vendorId,
                            token: s,
                            subscriptionJSON: e.toJSON()
                        } ).then( function () {
                            window.subscribersApp.trackRefreshed( s ), localStorage.setItem( "firebaseToken", s )
                        } )
                    } )
                } )[ "catch" ]( function ( e ) {
                    console.log( "Unable to retrieve refreshed token ", e ), window.subscribersApp.trackError( "Unable to retrieve refreshed token" )
                } )
            } ), navigator.serviceWorker.register( window.subscribersApp.serviceWorkerPath ).then( function ( s ) {
                e.useServiceWorker( s )
            } ).then( function () {
                return e.requestPermission()
            } ).then( function () {
                return e.getToken()
            } ).then( function ( r ) {
                r && "" !== r ? e.registrationToUse.pushManager.getSubscription().then( function ( e ) {
                    s( { token: r, subscriptionJSON: e.toJSON() } )
                } )[ "catch" ]( function () {
                    s( { token: r } )
                } ) : i( "Token is null" )
            } )[ "catch" ]( function ( e ) {
                var s = "messaging/permission-default" === e.code, r = "messaging/permission-blocked" === e.code,
                    n = "messaging/unsupported-browser" === e.code;
                s || r ? i( "denied" ) : n || i( e )
            } )
        } )[ "catch" ]( function ( e ) {
            i( Error( "loading Firebase error: " + e ) )
        } )
    } ) 
}

Notice the navigator.serviceWorker.register() here. This is a conflict with the service worker registered in the PWA plugin.

Really what needs to happen is:

  1. navigator.serviceWorker.register( window.subscribersApp.serviceWorkerPath ) needs to be replaced with navigator.serviceWorker.getRegistration().
  2. The logic from the service worker (currently loaded from /?firebase-messaging-sw) needs to be injected into the WordPress-registered service worker with something like this:
add_action(
	'wp_front_service_worker',
	function ( \WP_Service_Worker_Scripts $scripts ) {
		$scripts->register(
			'subscribers-com',
			function () {
				return '
					var version = "1.5";
					importScripts("https://cdn.subscribers.com/assets/subscribers-sw.js");
				';
			}
		);
	}
);

(Aside: I'm a bit concerned by the global version variable it depends on.)

I'll contact their support to see if they can add compatibility in their plugin.

@KZeni
Copy link
Contributor Author

KZeni commented Aug 1, 2019

That all makes sense. Thanks for digging into this and reaching out to Subscribers.com to get them to address their compatibility issue!

@westonruter
Copy link
Collaborator

I've opened a PR to fix this and have a build for testing: #279 (comment).

@westonruter westonruter added this to the 0.5 milestone May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants