Closed Bug 1416872 Opened 2 years ago Closed 2 years ago

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

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox58 fixed, firefox59 fixed)

VERIFIED FIXED
mozilla59
Tracking Status
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: rhubscher, Assigned: mixedpuppy)

References

Details

Attachments

(1 file)

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
Priority: -- → P1
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).
Attachment #8928327 - Flags: review?(lgreco) → review+
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 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 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 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).
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 :)
Flags: needinfo?(rhubscher)
Yes sure let me know :)
https://hg.mozilla.org/mozilla-central/rev/de5adc2a13ac
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
I confirm that with my last Nightly update it is back working as expected.
Status: RESOLVED → VERIFIED
Flags: needinfo?(rhubscher)
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.
Attachment #8928327 - Flags: approval-mozilla-beta?
Comment on attachment 8928327 [details]
Bug 1416872 fix canceling request,

Fix an identity with e10s issue and was verified. Beta58+.
Attachment #8928327 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.