Bitwarden widget not shown on webpages, shows only on about pages

[srirambv]

Test plan

#10197 (comment)

---

• Did you search for similar issues before submitting this one?
  Yes

• Describe the issue you encountered:
  Bitwarden widget not shown on webpages, shows only on about pages

• Platform (Win7, 8, 10? macOS? Linux distro?):
  Windows 10 x64

• Brave Version (revision SHA):
  Brave 0.17.1
  rev 84dbc8e
  Muon 4.0.3

• Steps to reproduce:

  1. Clean install 0.17.1
  2. Enable bitwarden from about:preferences#extensions, wait till the widget is shown
  3. Click on the widget, home screen is shown
  4. Open a new tab and visit gmail.com, widget is not shown, hovering over the empty space shows the tooltip for the widget

• Actual result:
  Bitwarden widget not shown on webpages, shows only on about pages

• Expected result:
  Widget should be shown on all pages

• Will the steps above reproduce in a fresh profile? If not what other info can be added?
  Yes

• Is this an issue in the currently released version?
  No

• Can this issue be consistently reproduced?
  Yes

• Extra QA steps:
  1.
  2.
  3.

• Screenshot if needed:
  

• Any related issues:
  cc: @kspearrin @bsclifton

[kspearrin]

I have no idea...

[jonathansampson]

Extensions have an associated tab collection, full of ids. This collection is consulted when switching tabs. @cezaraugusto, I would start searching there.

[cezaraugusto]

there's something affecting all extensions, other pw managers show the icon but are unclickable

[cezaraugusto]

this seems to be related to muon. I did a time-travel over more than 300 commits and issue appeared intermittent, but extension is always unclickable. I often get this in response:

Cannot read property 'populate' of null
    at EventEmitter.<anonymous> (/Users/cezaraugusto/dev/browser-laptop/node_modules/electron-prebuilt/dist/Brave.app/Contents/Resources/electron.asar/browser/api/extensions.js:579:49)

/cc @jonathansampson @kevinlawler

[bridiver]

from window_bindings.js

apiFunctions.setHandleRequest(‘getCurrent’, function () {
    var cb, getInfo
    if (arguments.length === 1) {
      cb = arguments[0]
    } else {
      getInfo = arguments[0]
      cb = arguments[1]
    }

if arguments.length === 1 getInfo will be null which would cause the error for getInfo.populate in extensions.js

[bbondy]

didn't make it in time for 0.17.x, sorry folks. Moved to 0.18.x.

[bsclifton]

cc: @kspearrin

We're going to move our target for Bitwarden to 0.18.x (milestones here; no date is assigned yet)

[kspearrin]

@bsclifton Ok. Let me know if there is anything I can do to help.

[bbondy]

Leaving it in as is for now, just disabling it in the 0.17.x branch for that release.

[jonathansampson]

The problem here is with an unpopulated path object:

[kspearrin]

@jonathansampson Is this something I need to fix in the extension or..?

[jonathansampson]

@kspearrin Not sure at the moment. The only bitwarden call I see to chrome.browserAction.setIcon provides a path object (for a 19 and 38 px image). For some reason, this path object is abandoned or lost some how.

I hope to identify the root cause here shortly.

[jonathansampson]

I'm looking at this again now, and am still a bit perplexed.

The extension calls chrome.browserAction.setIcon which invokes our AppStoreRenderer set state method. If you pause execution here, you'll find that the browserAction.path exists, and is populated with the data passed through chrome.browserAction.setIcon.

Following the execution further, we eventually get to the extensionState browserActionBackgroundImage method, where browserAction.path no longer exists. I suspect we're dropping the value somewhere in between. With a little more searching, I'm sure I'll find the issue.

I'll keep this thread updated with any roaming ideas I may have along the way.

[NejcZdovc]

@jonathansampson I think that I found the problem. So when you get browser actions here https://github.com/brave/browser-laptop/blob/master/app/renderer/components/navigation/browserAction.js#L58 everything is fine. The problem starts here https://github.com/brave/browser-laptop/blob/master/app/renderer/components/navigation/browserAction.js#L64 because we are replacing browserActions with tabAction. With this we lose path and color. I think that we just need to merge these two and we are good to go.

Fix that works for me:

diff --git a/app/renderer/components/navigation/browserAction.js b/app/renderer/components/navigation/browserAction.js
index 042387c20..85ec5b97b 100644
--- a/app/renderer/components/navigation/browserAction.js
+++ b/app/renderer/components/navigation/browserAction.js
@@ -61,6 +61,8 @@ class BrowserAction extends React.Component {
     if (tabAction) {
       tabAction = tabAction.set('title', browserActions.get('title'))
       tabAction = tabAction.set('base_path', browserActions.get('base_path'))
+      tabAction = tabAction.set('color', browserActions.get('color'))
+      tabAction = tabAction.set('path', browserActions.get('path'))
       browserActions = tabAction
     }

[jonathansampson]

@NejcZdovc That looks right. Might even be worth replacing the 4 calls to browserActions.get and 4 calls to tabAction.set with a single browserActions = tabAction.merge(browserActions). Thoughts?

[NejcZdovc]

That was my initial though as well, but I wasn't sure if that would be ok, because I am not familiar with tabAction vs browserAction.

[jonathansampson]

@NejcZdovc I took a quick look at the two, and they seem simple enough. It may be computationally better to merge than to make multiple get/set trips. The resulting object is still very small.

[NejcZdovc]

what I would do then is browserActions = browserActions.merge(tabAction) so that tabAction is overwriting browserActions.

[jonathansampson]

@NejcZdovc Done.