1608347 - Master password dialog pops up every time the browser starts when the site that requires login is at the top site

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Alice0775 White]

Attachment: screenshot

Reproducible: always, seems depend on Network speed and CPU spec.(maybe in case of low spec environment)

Steps to reproduce:
(prerequisite)

1. Saved login password for a site such as bugzilla.mozilla.org
2. Master password is registered
3. Disable "Always check if Firefox is your default browser" to avoid alert dialog if this is not default browser
4. Other than the above is all default settings of browser.

(steps)

1. Clear everything including history, cache, login cookies etc.
   Alt - History – Clear Recent History…
   Select Everything and check all checkbox and click [Clear Now]
2. Open https://bugzilla.mozilla.org/enter_bug.cgi
3. Quickly close Master Password dialog without enter password And quit browser
4. Start browser again. Observe the bug

Actual Results:
Tabbar only has a "New Tab Page" tab, but the Master Password dialog pops up

Expected Results:
the Master Password dialog should not pop up

After the Master Password pops up at step 2, then leave for a while (i.e.the top site thumbnail is successfully created).
Then step3-4, no longer reproduce the bug.
So, I think Master Password dialog should not offer during thumbnail construction.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2cd1797819f4395a460678e437b649ac75b5f557&tochange=98d1f98161e6656a2afcc791df87d027b44c55ce

Regressed by: Bug 1567175

Comment 2

[Neil Deakin]

For some reason 'https://bugzilla.mozilla.org/enter_bug.cgi' is being loaded into a document on startup at step 4 despite that it isn't being displayed in the tab. I don't have session restore enabled and fission is disabled.

I don't know why the bugzilla url would be loaded here. Perhaps there is some workaround that the login manager can do to avoid showing the master password prompt in this case, but we need to understand why the bugzilla url is being loaded when it isn't being displayed.

Comment 3

[Matthew N. [:MattN]]

It sounded to me like it's caused by thumbnail generation for the new tab page. In the past that used to load the URL into a hidden frame but I don't know how that works nowadays. I think the relevant code is https://searchfox.org/mozilla-central/rev/d4d6f81e0ab479cde192453bae83d5e3edfb39d6/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js#45-47 which already disables some features on the docShell.

Comment 4

[Neil Deakin]

OK, so I verified that it is indeed the thumbnail that is triggering the prompt. The prompt is coming up due to the DOMFormHasPassword/DOMInputPasswordAdded events being handled by the actor.

The non-fission compatible login manager handled these events in a frame script content.js. Those events I assume would have still been firing, but the frame script event listeners either aren't added to the bugzilla url document or don't fire.

So the login manager could check if the page has some state that indicates that this is a background loading image (which state to use?) and return early, or a general-purpose flag could be added to actor creation declarations to avoid creating actors in this state.

Alternatively, maybe we shouldn't be processing or firing the events at all?

Any thoughts?

Comment 5

[Matthew N. [:MattN]]

(In reply to Neil Deakin from comment #4)

The non-fission compatible login manager handled these events in a frame script content.js. Those events I assume would have still been firing, but the frame script event listeners either aren't added to the bugzilla url document or don't fire.

There used to a frame scripts that were loaded in all frames vs. ones that were loaded in the "browsers" group. The latter were only for tabs, not sidebars or other frames like the screenshot one. It's weird that the pwmgr ones were in content.js, not tab-content.js but the issue didn't happen before.

So the login manager could check if the page has some state that indicates that this is a background loading image

This could easily be done in shouldIgnoreLoginManagerEvent.

(which state to use?) and return early,

I wonder if the thumbnailer should be in its own content process and then we could use WindowActorOptions.remoteTypes.

We could introduce a new docShell boolean flag like the others we have. I'm not sure who else would use it though… maybe the webext frames?

Maybe we could re-use @messagemanagergroup and put this frame in its own group at https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/toolkit/components/thumbnails/BackgroundPageThumbs.jsm#308-311 though (though I realize we a moving away from message managers I think the concept may stick around).

Probably the easiest option would be to use its special userContextId and have shouldIgnoreLoginManagerEvent check for that, if possible.

or a general-purpose flag could be added to actor creation declarations to avoid creating actors in this state.

That sounds like bug 1557118: https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/browser/actors/BrowserTabParent.jsm#21-22 though in the past we supported login autofill in sidebars so perhaps it would have to be a sequence of groups supported (like remoteTypes is a sequence of types)

Alternatively, maybe we shouldn't be processing or firing the events at all?

I would prefer to keep this handling closer to pwmgr code so I would rather not change the firing but not processing them (like the shouldIgnoreLoginManagerEvent suggestion) is ideal IMO. I definitely think we need to stop all processing vs. just preventing the dialog since this is a privacy issue with usernames potentially being autofilled and tracking scripts reading it (we know this happens in the wild). Note that docShell.useTrackingProtection = true; mitigates this somewhat for 3rd-party trackers but not 1st parties.

Any thoughts?

I'm surprised that docShell.allowAuth = false; isn't already being used. We could piggyback on that for the MP dialog but I think the better solution is not doing the earlier work. Maybe we should add that flag in this bug too?

Comment 6

[Neil Deakin]

We could introduce a new docShell boolean flag like the others we have. I'm not sure who else would use it though… maybe the webext frames?
Probably the easiest option would be to use its special userContextId and have shouldIgnoreLoginManagerEvent check for that, if possible.

The content process can't access the ContextualIdentityService.jsm module where those constants are defined, as that module is initialized with a file path.

I'm surprised that docShell.allowAuth = false; isn't already being used. We could piggyback on that for the MP dialog but I think the better solution is not doing the earlier work. Maybe we should add that flag in this bug too?

Re-using the allowAuth flag and checking for it seems to work. It doesn't look to be inherited into child docshells though in nsDocShell::SetDocLoaderParent, unlike the other 'allow' flags. I assume this is just an oversight.

Fixing and using allowAuth might be a suitable solution for now. The Background Page Thumbs module will need to be rewritten sometime soon for fission anyway. From what I can tell, it currently loads different icons from different domains from the child frame script expecting them to load in the same process, so instead the parent will need to make these different requests.

Comment 7

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

The priority flag is not set for this bug.
:MattN, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Neil Deakin]

Attachment: Bug 1608347, add a flag to block login prompts from being allowed in a browsing context, and use this to prevent prompts for background loaded page thumbnails, r=smaug

Comment 10

[Ethan Tseng [:ethan]]

Neil, do you have any update on the patch?

Comment 11

[Neil Deakin]

Attachment: Bug 1608347, don't apply login manager actor to thumbnail service, r=MattN

Comment 12

[Neil Deakin]

Sorry, I forgot to add the dependency on bug 1557118. The property for JSWindowActors is called 'messageManagerGroup' but I can rename it 'group' if desired.

Comment 13

[Matthew N. [:MattN]]

(In reply to Neil Deakin from comment #12)

Sorry, I forgot to add the dependency on bug 1557118. The property for JSWindowActors is called 'messageManagerGroup' but I can rename it 'group' if desired.

If it takes an array it seems like it group be plural: messageManagerGroups would be best IMO but I see that's what you did in the patch :)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/88c73be53b9f
don't apply login manager actor to thumbnail service, r=MattN

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/88c73be53b9f

Comment 16

[Alice0775 White]

The problem is no longer reproduced on Nightly76.0a1(20200403063228).