1430117 - Checking NS_BINDING_ABORTED to avoid dispatching navigate event

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P3)

Description

[Ricky Chien [:rickychien] (inactive)]

Follow-up for fixing the suggested solution from https://bugzilla.mozilla.org/show_bug.cgi?id=1409705#c10.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

If you'd like to try out the patch, please rebase on top of bug 1409705.

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1430117 - Checking NS_BINDING_ABORTED to avoid dispatching navigate event

Review commit: https://reviewboard.mozilla.org/r/212464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212464/

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8942183 [details]
Bug 1430117 - Checking NS_BINDING_ABORTED to avoid dispatching navigate event

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212464/diff/1-2/

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8942183 [details]
Bug 1430117 - Checking NS_BINDING_ABORTED to avoid dispatching navigate event

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212464/diff/2-3/

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8942183 [details]
Bug 1430117 - Checking NS_BINDING_ABORTED to avoid dispatching navigate event

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212464/diff/3-4/

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8942183 [details]
Bug 1430117 - Checking NS_BINDING_ABORTED to avoid dispatching navigate event

https://reviewboard.mozilla.org/r/212464/#review218534

Thanks for the patch, it would have been great to have a test, but it seems to complex to catch the actor error and make the test fail.

::: devtools/server/actors/tab.js:1639
(Diff revision 4)
>        this._tabActor._willNavigate(window, newURI, request);
>      }
>      if (isWindow && isStop) {
>        // Don't dispatch "navigate" event just yet when there is a redirect to
>        // about:neterror page.
> -      if (request.status != Cr.NS_OK) {
> +      if (request.status != Cr.NS_OK && request.status != Cr.NS_BINDING_ABORTED) {

Could you add a comment explaining the NS_BINDING_ABORTED, something like:
 // Navigating to about:neterror will make `status` be something else than NS_OK. But for some error like NS_BINDING_ABORTED we don't want to emit any `navigate` event as the page load has been cancelled and the related page document is going to be a dead wrapper.

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8942183 [details]
Bug 1430117 - Checking NS_BINDING_ABORTED to avoid dispatching navigate event

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212464/diff/4-5/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27e396761ed3
Checking NS_BINDING_ABORTED to avoid dispatching navigate event r=ochameau

Comment 9

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/27e396761ed3