[E10S] FxA Private browsing mode query does not work with E10s enabled

VERIFIED FIXED in Firefox 55

Status

()

P2
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: stomlinson, Assigned: stomlinson)

Tracking

unspecified
Firefox 56
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 verified, firefox56 verified)

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

a year ago
This was first reported as an fxa-content-server issue in [1].

With E10s enabled, the private browsing mode query [2] errors with "Unable to check for private browsing mode, assuming true."

The result is that users who attempt to sign in to an OAuth relier must re-enter both their email and password.

STR

1. Start with a fresh profile and Fx >= 56 (though, I assume this should also be problematic on Fx 55)
2. Sign into Sync
3. Visit addons.mozilla.org
4. Click "Sign in"
5. Notice the FxA sign in screen. It's broken.

Expected
On the FxA signin screen, the email from step 2 should be displayed and non-editable. The user should not need to enter their password to sign in.

Actual
The user must enter both their email and password to sign in.


[1] - https://github.com/mozilla/fxa-content-server/issues/5209.
[2] - https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/services/fxaccounts/FxAccountsWebChannel.jsm#350
(Assignee)

Comment 1

a year ago
With e10s enabled, the check for sendingContext.browser.docShell returns null!

I just applied this patch:


---------------------------------

diff --git a/services/fxaccounts/FxAccountsWebChannel.jsm b/services/fxaccounts/FxAccountsWebChannel.jsm
--- a/services/fxaccounts/FxAccountsWebChannel.jsm
+++ b/services/fxaccounts/FxAccountsWebChannel.jsm
@@ -338,16 +338,32 @@ this.FxAccountsWebChannelHelpers.prototy
       return null;
     });
   },
 
   /**
    * Check if `sendingContext` is in private browsing mode.
    */
   isPrivateBrowsingMode(sendingContext) {
+    if (! sendingContext) {
+      log.error("sendingContext: Unable to check for private browsing mode, assuming true");
+      return true;
+    }
+    if (! sendingContext.browser) {
+      log.error("sendingContext.browser: Unable to check for private browsing mode, assuming true");
+      return true;
+    }
+    if (! sendingContext.browser.docShell) {
+      log.error(`sendingContext.browser.docShell: ${typeof sendingContext.browser.docShell} - {sendingContext.browser.docShell} - Unable to check for private browsing mode, assuming true`);
+      return true;
+    }
+    if (sendingContext.browser.docShell.usePrivateBrowsing === undefined) {
+      log.error("sendingContext.browser.docShell.usePrivateBrowsing: Unable to check for private browsing mode, assuming true");
+      return true;
+    }
     if (!sendingContext ||
         !sendingContext.browser ||
         !sendingContext.browser.docShell ||
         sendingContext.browser.docShell.usePrivateBrowsing === undefined) {
       log.error("Unable to check for private browsing mode, assuming true");
       return true;
     }
 

---------------------------------
 
When run and visiting `addons.mozilla.org`, I see this in the console:

> 1499356088251	FirefoxAccounts	ERROR	sendingContext.browser.docShell: object - null - Unable to check for private browsing mode, assuming true
Priority: -- → P2
I haven't dug too far into this, but it seems likely that the calls to webchannel listeners are being remoted from the parent to a child process and that this is causing the docShell to be null as they are unable to be remoted. If that's true, it might be necessary to explicitly flag privateBrowsing as the sendingContext is created.

I guess the first step though it still to work out if that is indeed what is happening.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → stomlinson
(Assignee)

Comment 4

a year ago
:markh, :eoger - TIL there is a module [1] that makes it easy to determine whether the WebChannel message's sendingContext is in private browsing mode! I opened a patch with a fix because this problem is starting to show up all over the place. If the attached fix is approved, is there any chance this could be uplifted to Fx 55? If not, we are going to have to work around the breakage from the content server side.


[1] - https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/PrivateBrowsingUtils.jsm

Comment 6

a year ago
mozreview-review
Comment on attachment 8885777 [details]
Fix the private browsing mode check in FxAccountsWebChannel.jsm (bug 1378766)

https://reviewboard.mozilla.org/r/156572/#review161706

lgtm

::: services/fxaccounts/FxAccountsWebChannel.jsm:349
(Diff revision 1)
>  
>    /**
>     * Check if `sendingContext` is in private browsing mode.
>     */
>    isPrivateBrowsingMode(sendingContext) {
>      if (!sendingContext ||

Can browser be actually undefined? If it does I'm thinking that test should be done earlier in the call stack with a different error message, if not just remove the whole test.
Attachment #8885777 - Flags: review?(eoger) → review+

Comment 7

a year ago
mozreview-review
Comment on attachment 8885777 [details]
Fix the private browsing mode check in FxAccountsWebChannel.jsm (bug 1378766)

https://reviewboard.mozilla.org/r/156572/#review161874

LGTM, thanks!
Attachment #8885777 - Flags: review?(markh) → review+
(Assignee)

Comment 8

a year ago
mozreview-review-reply
Comment on attachment 8885777 [details]
Fix the private browsing mode check in FxAccountsWebChannel.jsm (bug 1378766)

https://reviewboard.mozilla.org/r/156572/#review161706

> Can browser be actually undefined? If it does I'm thinking that test should be done earlier in the call stack with a different error message, if not just remove the whole test.

I don't know whether browser can actually be undefined. :markh, do you know?

Comment 9

a year ago
mozreview-review-reply
Comment on attachment 8885777 [details]
Fix the private browsing mode check in FxAccountsWebChannel.jsm (bug 1378766)

https://reviewboard.mozilla.org/r/156572/#review161706

> I don't know whether browser can actually be undefined. :markh, do you know?

I believe browser will always be defined
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
[Tracking Requested - why for this release]:
status-firefox55: --- → affected
status-firefox56: --- → affected
Keywords: checkin-needed

Comment 12

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b08d3ba207d5
Fix the private browsing mode check in FxAccountsWebChannel.jsm r=eoger,markh
Keywords: checkin-needed
Shane, can you please click on "details" for the attachment, then select approval‑mozilla‑beta=? and fill out the form there?
Flags: needinfo?(stomlinson)

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b08d3ba207d5
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 15

a year ago
Comment on attachment 8885777 [details]
Fix the private browsing mode check in FxAccountsWebChannel.jsm (bug 1378766)

Approval Request Comment
[Feature/Bug causing the regression]: 
* bz 1378766
* https://github.com/mozilla/fxa-content-server/issues/5209

[User impact if declined]:
Users who previously signed into FxA for Sync and attempt to sign into an OAuth relier such as AMO are asked for both their username and password when they should not need to fill out either.

[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]: 
I'd be happy for a manual test.

1. Using Fx Desktop Beta with E10s enabled, open "about:preferences#sync"
2. Click "Sign in", sign in to FxA with an existing account. Confirm signin if necessary.
3. Using the same browser, open https://addons.mozilla.org.
4. Click "Log in"
5. When FxA opens, the user should just be able to click "Sign in" without entering their email/password, but this does not work.

6. Repeat steps 1-5 with Nightly. At step 5, the user should be able to click "Sign in" without any additional input.

[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
I do not believe this change to be risky because we replaced a homegrown check for whether a window is in E10s mode to using a supported module (PrivateBrowsingUtils.jsm) that already ships in browser code.

[String changes made/needed]:
None
Flags: needinfo?(stomlinson)
Attachment #8885777 - Flags: approval-mozilla-beta?
(In reply to Shane Tomlinson [:stomlinson] from comment #15)
> [Feature/Bug causing the regression]: 
> * bz 1378766
> * https://github.com/mozilla/fxa-content-server/issues/5209
> 
This question was about when the bug was introduced, not when it was noticed/fixed :)
I guess that's bug 1308038.
Blocks: 1308038
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected
Comment on attachment 8885777 [details]
Fix the private browsing mode check in FxAccountsWebChannel.jsm (bug 1378766)

fix private browsing check for FxA, should be in 55.0b12
Attachment #8885777 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 18

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/ed08245d5233
status-firefox55: affected → fixed
Flags: in-testsuite+
Flags: qe-verify+
I've tested on Windows 7 x64, Ubuntu 14.04 x64 and Mac OS X 10.12.5 using latest Nightly 56.0a1 (2017-07-26) and Firefox 55 Beta 12 (20170724055319) and I have the following mentions: 
- everything looks good in normal browsing (normal window)
- in a private window, after login, if I open addons.mozilla.org, I need to re-enter email/password. It is expected behaviour here?
Flags: needinfo?(stomlinson)
(Assignee)

Comment 20

a year ago
(In reply to Camelia Badau, QA [:cbadau] from comment #19)
> I've tested on Windows 7 x64, Ubuntu 14.04 x64 and Mac OS X 10.12.5 using
> latest Nightly 56.0a1 (2017-07-26) and Firefox 55 Beta 12 (20170724055319)
> and I have the following mentions: 
> - everything looks good in normal browsing (normal window)
> - in a private window, after login, if I open addons.mozilla.org, I need to
> re-enter email/password. It is expected behaviour here?

Thank you for checking, yes, this is the expected behavior. 

We took this approach to try to cause the fewest user surprises.

User info is returned in private browsing mode to FxA only if the user is 
signing into Sync. We take this approach because Sync is viewed as an 
integral part of the browser, and we should never break the browser.

OAuth reliers on the other hand are not considered an integral part
of the browser. If the browser shared user info with FxA in PB mode
for say, AMO, that might surprise users. We are trying to avoid that
surprise.
Flags: needinfo?(stomlinson)
Marking the issue as VERIFIED FIXED on Firefox 56 and Firefox 55 based on comment 20.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
status-firefox56: fixed → verified
Flags: qe-verify+

Updated

a year ago
Product: Core → Firefox
Target Milestone: mozilla56 → Firefox 56
You need to log in before you can comment on or make changes to this bug.