1416872 - browser.identity.launchWebAuthFlow doesn't catch the last redirection anymore.

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[Rémy Hubscher (:natim)]

Since Bug 1411646 was fixed I realized that we can't use the launchWebFlow anymore.

Basically the flow redirect to the domain name and then the popup doesn't close itself anymore.

Mozregression gave me: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d2226bec0730a0e234d21b04d94677e49e4ff1ac&tochange=d32ac3e582b643d0938a3f094dd1b9d31dc8f065

You can reproduce the error using the notes plugin on the sync-v4 branch (npm i and then load the manifest.json in about:debugging) [0]

[0] https://github.com/mozilla/notes

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1416872 fix canceling request,

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

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8928327 [details]
Bug 1416872 fix canceling request,

https://reviewboard.mozilla.org/r/199536/#review205028

Like briefly discussed over IRC, the fix looks good, but we are still concerned that the test case wasn't able to catch this issue, and so we would like to tweak the test to be able to catch this regression (in this patch if it is possible, or in a follow up issue in the worst case).

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8928327 [details]
Bug 1416872 fix canceling request,

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

Comment 4

[Shane Caraveo (:mixedpuppy)]

Ok, the test was only running in-process so we didn't catch the bug.  This moves the test so it runs both in and oop.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8928327 [details]
Bug 1416872 fix canceling request,

https://reviewboard.mozilla.org/r/199534/#review205132

::: toolkit/components/extensions/test/mochitest/chrome.ini:12
(Diff revision 2)
>    file_image_great.png
>    file_sample.html
>    file_with_images.html
>    webrequest_chromeworker.js
>    webrequest_test.jsm
>    oauth.html

I think that this "oauth.html" support-file also needs to be moved to mochitest.ini (or added also in mochitest.ini if there are mochitest-chrome tests that needs oauth.html)

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8928327 [details]
Bug 1416872 fix canceling request,

https://reviewboard.mozilla.org/r/199534/#review205134

::: toolkit/components/extensions/test/mochitest/test_ext_identity.html:101
(Diff revision 2)
>          "identity",
>          "https://example.com/",
>        ],
>      },
>      async background() {
>        let base_uri = "https://example.com/chrome/toolkit/components/extensions/test/mochitest/";

This (as well as the other "https://example.com/..." urls at line 123 and line 138) should be probably "https://example.com/tests/toolkit/components/extensions/test/mochitest/"
now that this tests is a mochitest-plain test instead of a mochitest-chrome test.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8928327 [details]
Bug 1416872 fix canceling request,

https://reviewboard.mozilla.org/r/199536/#review205374

::: toolkit/components/extensions/ext-identity.js:74
(Diff revision 2)
>  
>      wpl = {
>        onStateChange(progress, request, flags, status) {
> -        if (request instanceof Ci.nsIHttpChannel &&
> +        if (request && request.URI &&
>            request.URI.spec.startsWith(redirectURI)) {
> -          request.cancel(Components.results.NS_BINDING_ABORTED);
> +          window.gBrowser.webNavigation.stop(Ci.nsIWebNavigation.STOP_ALL);

I think that it could be worth to add an inline comment here, to mention that the request object here is a  `RemoteWebProgressRequest` (as defined in RemoteWebProgress.jsm, https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/toolkit/modules/RemoteWebProgress.jsm#19-32)

and so we cannot use `request.cancel` to cancel the request, and we use `window.gBrowser.webNavigation.stop`, where webNavigation is an instance of `RemoteWebNavigation` (as defined in RemoveWebNavigation.js, https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/toolkit/components/remotebrowserutils/RemoteWebNavigation.js#27).

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8928327 [details]
Bug 1416872 fix canceling request,

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de5adc2a13ac
fix canceling request, r=rpl

Comment 10

[Shane Caraveo (:mixedpuppy)]

Rémy, when this lands can you give it a spin with notes?  The test is updated to catch the problem and I've tested with notes, but external validation is nice :)

Comment 11

[Rémy Hubscher (:natim)]

Yes sure let me know :)

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/de5adc2a13ac

Comment 13

[Rémy Hubscher (:natim)]

I confirm that with my last Nightly update it is back working as expected.

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8928327 [details]
Bug 1416872 fix canceling request,

Approval Request Comment
[Feature/Bug causing the regression]: 1411646 
[User impact if declined]: identity api will not work with e10s
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: small change, and test is fixed to run with e10s/oop
[String changes made/needed]: none

Having the test for this run under mochitest-chrome resulted in missing the failure the prior patch caused.  test is moved to mochitest-plain where it did catch that failure.

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8928327 [details]
Bug 1416872 fix canceling request,

Fix an identity with e10s issue and was verified. Beta58+.

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/18de3e3278b7