1748158 - Regression - Lastpass blank in private window

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P2)

Description

[Patrick (he/him)]

Attachment: Private Window

Steps to reproduce:

1. Install Lastpass, and allow to run in private window
2. Sign into Lastpass
3. Open a private browsing window
4. Navigate to a site where you have credentials saved
5. Click Lastpass icon
6. Accept Permissions

Expected results:
Lastpass dialog appears

Actual results:
Dialog blank

Reproducible in Firefox 95.0.2 and Nightly 97.0a1 (2022-01-01). Reproducible in a fresh profile.
Have run mozregression, appears to be regression of 1708243:
Bug 1708243 - Part 2: stop using sender data from the child process r=robwu,agi

Differential Revision: https://phabricator.services.mozilla.com/D123351

Comment 1

[Patrick (he/him)]

Attachment: Normal Window

Comment 2

[kernp25]

Attachment: QhRSEqlsM0.png

Does it work, if you turn this setting off?

I need to create an account, in order to test it.

Comment 3

[kernp25]

Attachment: firefox_1S9mcoymbj.png

I'm seeing this error in the console.

Comment 4

[kernp25]

The code from LastPass:

chrome.runtime.onConnect.addListener(function(n) {
   if (0 === n.name.indexOf("requestPort")) {
      var e = c.initializeRequestFramework({
         sendContentScript: function(e) {
            n.postMessage(e)
         },
         tabDetails: o(n.sender && n.sender.tab),
         frameIdentity: n.sender && n.sender.frameId ? n.sender.tab.id + "-" + n.sender.frameId : null
      });
      n.onMessage.addListener(e.requestHandler), n.onDisconnect.addListener(function() {
         e.disconnectHandler()
      })
   }
})

Comment 5

[kernp25]

Attachment: firefox_M1Jb7YMwIW.png

The tab is undefined, because the actor is null.

Comment 6

[kernp25]

I found this page: https://community.logmein.com/t5/LastPass-Support-Discussions/LastPass-extension-window-is-blank-in-Firefox-Private-tabs/m-p/277225

Reporter, can you try https://bugzilla.mozilla.org/show_bug.cgi?id=1748158#c2?

Comment 7

[Patrick (he/him)]

Changing the ETP settings does not fix the issue.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1708243

Comment 9

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

(In reply to kernp25 from comment #4)

The code from LastPass:

chrome.runtime.onConnect.addListener(function(n) {
   if (0 === n.name.indexOf("requestPort")) {
      var e = c.initializeRequestFramework({
         sendContentScript: function(e) {
            n.postMessage(e)
         },
         tabDetails: o(n.sender && n.sender.tab),
         frameIdentity: n.sender && n.sender.frameId ? n.sender.tab.id + "-" + n.sender.frameId : null
      });
      n.onMessage.addListener(e.requestHandler), n.onDisconnect.addListener(function() {
         e.disconnectHandler()
      })
   }
})

I see what is going on here:

• the exception is getting raised when the lastpass code snipped above is handling a "requestPort" runtime message sent from backgroundFrame.html (which is an iframe being created dinamically in the lastpass extension background page)
• lastpass code snipped above is clearly assuming that sender.tab should be defined if sender.frameId is not undefined
• I verified locally that:
  • before the changes introduced from Bug 1708243 (in Firefox 93), sender.frameId value was set to undefined (and sender.tab not part of the sender object properties at all)
  • currently, sender.frameId is set but sender.tab is not (which is expected because "backgroundFrame.html" isn't an extension page loaded in a tab anyway)

As next steps:

• we may want to evaluate how useful the new behavior may be in practice: with the Extensions APIs currently provided, I'm not really sure that providing the sender.frameId may be actually useful without a sender.tab.id, there isn't much that the extension can do with that frameId (can likely retrieve some more details using webNavigation API, and more recently comparing that frameId with the one that a content script may get using browser.runtime.getFrameId)

• Fix issue on the Firefox side by restoring the previous behavior: (if we are going to agree that including sender.frameId is never going to be useful) we could make sure to set sender.frameId to undefined if sender.tab isn't set to restore the previous behavior

• Fix issue on lastpass extension side: the lastpass could prevent the issue on the extension side by also checking if sender.tab is set along with checking the sender.frameId (which may be reasonable, given that the current behavior have been around from Firefox >= 93, and so users on older Firefox versions would still be affected by this issue even if fixed in more recent Firefox versions)

• **evaluate impact on cross-browser compatibility **: I haven't checked the behavior on Google Chrome, to confirm if Chrome does also set frameId to undefined in these cases, but it seems likely that it does (at least from a quick look to the deminified lastpass code I don't see the code around those checks to include any browser-vendor related special cases), we may want to confirm that (to evaluate the impact of our final choice in terms of cross-browser compatibility).

Comment 10

[kernp25]

Attachment: rjImngdaKU.png

LastPass in a private window

Comment 11

[kernp25]

Changing the code:

frameIdentity: n.sender && n.sender.frameId ? n.sender.tab.id + "-" + n.sender.frameId : null

to

frameIdentity: n.sender && n.sender.tab && n.sender.frameId ? n.sender.tab.id + "-" + n.sender.frameId : null

will fix the blank popup issue.

Comment 12

[Rob Wu [:robwu]]

This bug is happening because we are unconditionally setting frameId, while the previous implementation matched the documented behavior, i.e. that frameId is unset if tab is unset:

frameId Optional
integer. The frame that opened the connection. Zero for top-level frames, positive for child frames. This will only be set when tab is set.

- https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/MessageSender

It's straightforward to fix the implementation, but as I've taken a closer look, a little more effort is needed to fix unit tests with incorrect expectations/assumptions.

Comment 13

[Rob Wu [:robwu]]

Attachment: Bug 1748158 - Omit sender.frameId if sender.tab is unset

sender.frameId should be set iff sender.tab is set, as documented at
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/MessageSender
The removal of the viewType == "tab" condition broke this in
https://hg.mozilla.org/mozilla-central/rev/2dc4f1baccc8

This patch makes the presence of frameId conditional on tab, and
fixes several tests that relied on the incorrect behavior:

• Move the runtime.onConnect test from test_ext_contentscript_in_background.js
  to a new mochitest at test_ext_runtime_connect_iframe.html.

• Simplify test_ext_contentscript_in_background.js to continue to
  provide test coverage for contentScripts.register + allFrames.

• Replace runtime.onConnect with runtime.getFrameId in
  test_ext_contentscript_xorigin_frame.js, since sender.frameId is no
  longer available in xpcshell tests (because internals to support the
  tabs extension API are not available in xpcshell tests). The test
  cannot be moved to a mochitest because its purpose is to provide test
  coverage for process switching in a xpcshell test (bug 1580811).

Comment 14

[Rob Wu [:robwu]]

Some more details on how this extraneous frameId field breaks Lastpass:

1. Lastpass uses chrome.extension.getBackgroundPage() to get a reference to the background script to support its functionality. In private browsing mode, the popup window is hosted in a private browsing window, so the panel cannot get a direct reference to the extension's background page (unless the extension uses "incognito":"split", which it should, but it does not). When Lastpass detects this situation, it creates an iframe as a fallback, and uses that to communicate between the popup page and the background script.
2. Lastpass's background script uses an runtime.onConnect listener that expects port.sender.tab to be set for messages from frames (identified by port.sender.frameId). Popup panels are not tabs, so a runtime error is thrown (comment 3) and Lastpass fails to establish a communication channel between the popup page and the background (comment 4), and consequently the popup fails to initialize.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0bf0fa6d8a5a
Omit sender.frameId if sender.tab is unset r=rpl

Comment 16

[Marian-Vasile Laza]

Backed out changeset 0bf0fa6d8a5a (Bug 1748158) for causing xpcshell failures on test_ext_native_messaging_permissions.js.
Backout link
Push with failures
Failure Log

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ae7738694e1
Omit sender.frameId if sender.tab is unset r=rpl,geckoview-reviewers,jonalmeida

Comment 18

[Alexandru Michis [:malexandru]]

Backed out changeset 4ae7738694e1 (Bug 1748158) for causing xpcshell failures in test_ext_native_messaging_permissions.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/3321b8f93cb139ce47bbc0c2ed9c289677c3d6d0
Push with failures, failure log.

Comment 19

[Rob Wu [:robwu]]

There was another test expectation in that file that needed to be updated...

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7580bb764d4
Omit sender.frameId if sender.tab is unset r=rpl,geckoview-reviewers,jonalmeida

Comment 21

[Sandor Molnar[:smolnar]]

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

Comment 22

[kernp25]

Can this fix be uplifted to Firefox 96?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Unfortunately this landed too late for the Fx96 RC builds, but we are keeping it on the radar to hopefully ride along with a point release should the opportunity arise. In the mean time, verification that the fix works as expected on Nightly builds (or Beta 97 next week once those go out) would be very helpful.

Comment 24

[kernp25]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)

verification that the fix works as expected on Nightly builds (or Beta 97 next week once those go out) would be very helpful.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

You can help with that too, Patrick :)

Comment 26

[kernp25]

You need an account, in order to test it!

Comment 27

[kernp25]

Attachment: 5mj8tEIvR8.png

Comment 28

[kernp25]

I already tested it in 97.0a1 (2022-01-07) (64-Bit) and the fix works as expected!

Comment 29

[Alex Cornestean]

Hello,

Verified the fix on the latest Nightly (97.0a1/20220107093244) under Windows 10 x64 and Ubuntu 16.04 LTS following the initial STR.

The LastPass pop-up is no longer blank in Private Windows, confirming the fix. See the attached screenshot for further details.

Comment 30

[Alex Cornestean]

Attachment: 2022-01-07_16h25_32.png

Comment 31

[kernp25]

Did not know, you have a LastPass account :)

Comment 32

[Alex Cornestean]

(In reply to kernp25 from comment #31)

Did not know, you have a LastPass account :)

I created one some time back to test out an issue. But you beat me to the punch with the verification this time :).

Comment 33

[kernp25]

Can the needinfo be cleared?

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Sure, thanks for testing.

Comment 35

[Rob Wu [:robwu]]

Comment on attachment 9257515 [details]
Bug 1748158 - Omit sender.frameId if sender.tab is unset

Beta/Release Uplift Approval Request

• User impact if declined: Extensions that rely on the documented behavior of the sender.frameId property may fail, such as Lastpass in this bug report.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small change that hides a property in an API, to match the documented behavior of Firefox 92 and earlier (and Chromium).
• String changes made/needed:

Comment 36

[Dianna Smith [:diannaS]]

Comment on attachment 9257515 [details]
Bug 1748158 - Omit sender.frameId if sender.tab is unset

Approved by 96.0.2

Comment 37

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-release/rev/8d55b95f31d0

Comment 38

[Alex Cornestean]

Hello,

Verified the fix on the latest Beta (97.0b5/20220118185733) and Release (96.0.2/20220119190439) under Windows 10 x64 and Ubuntu 16.04 LTS following the initial STR.

The LastPass pop-up is no longer blank in Private Windows, confirming the fix. See the attached screenshot for further details.

Comment 39

[Alex Cornestean]

Attachment: 2022-01-20_10h00_00.png