bug(router-store): emitting guard results from state changes can cause infinite loops in certain circumstances

[damienwebdev]

Minimal reproduction of the bug/regression with instructions:

^v8.0.0 https://stackblitz.com/edit/angular-8-getting-started-1koz3g
^v10.0.0 https://stackblitz.com/edit/angular-ivy-bf2jo8

Expected behavior:

Not an infinite loop, instead a simple navigation cancel resulting in a white-screen.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

@ngrx/platform: v8.6.0
@angular/core: v8.0.0

Other information:

This exists in develop as well, but I simply made a repro on the version of Angular in which I found the bug.

The bug is a result of two things:

1. A guard on the root that utilizes state for its observable emission.
2. A race-condition here..

The router-store code, which is quite old (pre-ngrx 4), assumes that state reduction follows IMMEDIATELY after an action is dispatched. Yet, this isn't always the case, given that actions are stacked onto an rxjs queue. This means that when this check occurs the trigger value is RouterTrigger.NONE instead of RouterTrigger.ROUTER which results in an infinite loop as it triggers another navigation to the root re-checking the guard.

I would be willing to submit a PR to fix this issue

[x] Yes (Assistance is provided if you need help submitting a pull request)
[] No

[alex-okrushko]

Some details around this bug:

Current behavoir of Angular Router is to navigate to / (or root path) when guard returns false (or emits Observable/Promise with false) and does not provide any specific UrlTree.

When such Guard is guarding the root path itself, router-store reacts to the change in the store a bit later than it should, and misses the RouterTrigger.ROUTER (gets onto the state when it's cleared to RouterTrigger.NONE already).

@damienwebdev correctly identified the line:

platform/modules/router-store/src/router_store_module.ts

Line 214 in 1432891

if (this.trigger === RouterTrigger.ROUTER) {

The code then proceeds and checks this.router.url and the url that's passed as part of the state:

It mismatches and triggers another re-routing, which triggers another guard and so on, getting everything into the loop.

While not being the ideal solution, improving the matching of URLs solves the issue.
We might want to revisit the whole "keeping triggers and storeState" in class properties, and instead pass them into the function calls, but that would require a far bigger re-write.