Bugzilla

Currently, I'm looking at failures for [browser/components/urlbar/tests/browser/browser_stop_pending.js](https://treeherder.mozilla.org/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=26f794cd9036f6dd7cf0de7bee03def086ed11ca&selectedTaskRun=U8LB7SGER1eyLGFXjYjxGw.0). 


In the test, we first load example.com in a tab. Then, once the page has fully loaded, in the same tab we set the value of gURLBar to SLOW_PAGE [here](https://searchfox.org/mozilla-central/rev/95d8478112eecdd0ee249a941788e03f47df240b/browser/components/urlbar/tests/browser/browser_stop_pending.js#55), the request for which takes a really long time to complete (and never completes, explained later). Then after a 200ms timeout, we set the value of gURLBar to SLOW_PAGE2 [here](https://searchfox.org/mozilla-central/rev/95d8478112eecdd0ee249a941788e03f47df240b/browser/components/urlbar/tests/browser/browser_stop_pending.js#60). The test tries to assert that cancelling of the old load due to the current load does not reset the value of the url bar to the very first page we loaded - example.com. 

With the pref enabled and my other changes applied, the behaviour changes and the value of the url bar changes to example.com (despite the fact that we have another load happening), which I can also confirm visually when I run the test is headed mode.

This is happening because in TabProgressListener::onStateChange, upon receiving a STATE_STOP event with a status code that indicates an error and if we see that the browser's `isNavigating` value is false, we set the value of the url bar to browser's current URI (in our case example.com). So how does this happen?

When we start the load for SLOW_PAGE2, we set the value of isNavigating to true, then we cancel the other load (SLOW_PAGE), and set isNavigating to false. Cancelling SLOW_PAGE load results in a STATE_STOP event being fired, and that's where we decide to reset the value of the url bar.

Without the pref enabled, the timing of when the STATE_STOP gets fired and when the `isNavigating` is different, and thus we don't set the url bar to the previous value. (This part of the explanation will be a bit blurry) The value of `isNavigating` is set due to an IPC call, which arrives later, and when STATE_STOP event is received, we don't change the value of the url bar because isNavigating is set to true.

I came up with two solutions
1) we could look into setting isNavigating to false a bit later, not right after we are done executing the first method of loading a uri (as per description) but when the navigation itself completes, and do so while making sure that only current loads can set this value to false. This would require making sure that we set this value to false around the same time both with fission and without, and with this pref, and without this pref.

2) given that we have been [considering](https://searchfox.org/mozilla-central/rev/95d8478112eecdd0ee249a941788e03f47df240b/browser/base/content/browser.js#1539) getting rid of `isNavigating` field, we could go about this differently. In the cases when a load is cancelled due to another one starting, e.g. reload, going forward/back, going to index, etc, we could cancel the load using a distinct error code. Then, in TabProgressListener::onStateChange event, we could look at the status code and before resetting the url bar, check that the status code does not match that distinct error code. 

(todo write more about needing to add tests to cover this case, and auditing other uses of isNavigating)

Currently, I'm looking at failures for [browser/components/urlbar/tests/browser/browser_stop_pending.js](https://treeherder.mozilla.org/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=26f794cd9036f6dd7cf0de7bee03def086ed11ca&selectedTaskRun=U8LB7SGER1eyLGFXjYjxGw.0). 


In the test, we first load example.com in a tab. Then, once the page has fully loaded, in the same tab we set the value of gURLBar to SLOW_PAGE [here](https://searchfox.org/mozilla-central/rev/95d8478112eecdd0ee249a941788e03f47df240b/browser/components/urlbar/tests/browser/browser_stop_pending.js#55), the request for which takes a really long time to complete (and never completes, explained later). Then after a 200ms timeout, we set the value of gURLBar to SLOW_PAGE2 [here](https://searchfox.org/mozilla-central/rev/95d8478112eecdd0ee249a941788e03f47df240b/browser/components/urlbar/tests/browser/browser_stop_pending.js#60). The test tries to assert that cancelling of the old load due to the current load does not reset the value of the url bar to the very first page we loaded - example.com. 

With the pref enabled and my other changes applied, the behaviour changes and the value of the url bar changes to example.com (despite the fact that we have another load happening), which I can also confirm visually when I run the test is headed mode.

This is happening because in TabProgressListener::onStateChange, upon receiving a STATE_STOP event with a status code that indicates an error and if we see that the browser's `isNavigating` value is false, we set the value of the url bar to browser's current URI (in our case example.com). So how does this happen?

When we start the load for SLOW_PAGE2, we set the value of isNavigating to true, then we cancel the other load (SLOW_PAGE), and set isNavigating to false. Cancelling SLOW_PAGE load results in a STATE_STOP event being fired, and that's where we decide to reset the value of the url bar.

Without the pref enabled, the timing of when the STATE_STOP gets fired and when the `isNavigating` is different, and thus we don't set the url bar to the previous value. (This part of the explanation will be a bit blurry) The value of `isNavigating` is set due to an IPC call, which arrives later, and when STATE_STOP event is received, we don't change the value of the url bar because isNavigating is set to true.

I came up with two solutions
1) we could look into setting isNavigating to false a bit later, not right after we are done executing the first method of loading a uri (as per description) but when the navigation itself completes, and do so while making sure that only current loads can set this value to false. This would require making sure that we set this value to false around the same time both with fission and without, and with this pref, and without this pref.

2) given that we have been [considering](https://searchfox.org/mozilla-central/rev/95d8478112eecdd0ee249a941788e03f47df240b/browser/base/content/browser.js#1539) getting rid of `isNavigating` field, we could go about this differently. In the cases when a load is cancelled due to another one starting, e.g. reload, going forward/back, going to index, etc, we could cancel the load using a distinct error code. Then, in TabProgressListener::onStateChange event, we could look at the status code and before resetting the url bar, check that the status code does not match that distinct error code (instead of checking the browser's `isNavigating` value)

Solution 2 seems like a more reasonable solution (I mentioned it in a meeting to kmag, who agreed), because hopefully it will also allow us to remove `isNavigating` field, so this is what i'm going with. I will need to add more tests to make sure that the url bar does not reset in the aforementioned cases, e.g. reload, going forward/back, going to index. And most importantly, I will need to make sure that I audit other uses of isNavigating and look for a work around (perhaps this could be done in a follow up, and not as part of this work?) For now we could just get rid of  the usage of `isNavigating` in TabProgressListener::onStateChange

Currently, I'm looking at [failures for browser/components/urlbar/tests/browser/browser_stop_pending.js](https://treeherder.mozilla.org/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=26f794cd9036f6dd7cf0de7bee03def086ed11ca&selectedTaskRun=U8LB7SGER1eyLGFXjYjxGw.0). 


In the test, we first load example.com in a tab. Then, once the page has fully loaded, in the same tab we set the value of gURLBar to SLOW_PAGE [here](https://searchfox.org/mozilla-central/rev/95d8478112eecdd0ee249a941788e03f47df240b/browser/components/urlbar/tests/browser/browser_stop_pending.js#55), the request for which takes a really long time to complete (and never completes, explained later). Then after a 200ms timeout, we set the value of gURLBar to SLOW_PAGE2 [here](https://searchfox.org/mozilla-central/rev/95d8478112eecdd0ee249a941788e03f47df240b/browser/components/urlbar/tests/browser/browser_stop_pending.js#60). The test tries to assert that cancelling of the old load due to the current load does not reset the value of the url bar to the very first page we loaded - example.com. 

With the pref enabled and my other changes applied, the behaviour changes and the value of the url bar changes to example.com (despite the fact that we have another load happening), which I can also confirm visually when I run the test is headed mode.

This is happening because in TabProgressListener::onStateChange, upon receiving a STATE_STOP event with a status code that indicates an error and if we see that the browser's `isNavigating` value is false, we set the value of the url bar to browser's current URI (in our case example.com). So how does this happen?

When we start the load for SLOW_PAGE2, we set the value of isNavigating to true, then we cancel the other load (SLOW_PAGE), and set isNavigating to false. Cancelling SLOW_PAGE load results in a STATE_STOP event being fired, and that's where we decide to reset the value of the url bar.

Without the pref enabled, the timing of when the STATE_STOP gets fired and when the `isNavigating` is different, and thus we don't set the url bar to the previous value. (This part of the explanation will be a bit blurry) The value of `isNavigating` is set due to an IPC call, which arrives later, and when STATE_STOP event is received, we don't change the value of the url bar because isNavigating is set to true.

I came up with two solutions
1) we could look into setting isNavigating to false a bit later, not right after we are done executing the first method of loading a uri (as per description) but when the navigation itself completes, and do so while making sure that only current loads can set this value to false. This would require making sure that we set this value to false around the same time both with fission and without, and with this pref, and without this pref.

2) given that we have been [considering](https://searchfox.org/mozilla-central/rev/95d8478112eecdd0ee249a941788e03f47df240b/browser/base/content/browser.js#1539) getting rid of `isNavigating` field, we could go about this differently. In the cases when a load is cancelled due to another one starting, e.g. reload, going forward/back, going to index, etc, we could cancel the load using a distinct error code. Then, in TabProgressListener::onStateChange event, we could look at the status code and before resetting the url bar, check that the status code does not match that distinct error code (instead of checking the browser's `isNavigating` value)

Solution 2 seems like a more reasonable solution (I mentioned it in a meeting to kmag, who agreed), because hopefully it will also allow us to remove `isNavigating` field, so this is what i'm going with. I will need to add more tests to make sure that the url bar does not reset in the aforementioned cases, e.g. reload, going forward/back, going to index. And most importantly, I will need to make sure that I audit other uses of isNavigating and look for a work around (perhaps this could be done in a follow up, and not as part of this work?) For now we could just get rid of  the usage of `isNavigating` in TabProgressListener::onStateChange