1538460 - Tab detached unexpectedly after click on Master Password Dialog button

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Alice0775 White]

Reproducible: always

Steps To Reproduce:

1. Make sure master password set

2. Logout Bugzilla if any

3. Open Bugzilla link in background tab

4. Wait for tab loading

5. Mouse down on the background tab

6. And then mouse up at top border of URLbar
   (i.e. this should be within detachTabThresholdY, so tab detaching does never happen)

7. Click on Master Password Dialog button

Actual Results:
Tab detached unexpectedly....

Expected Results:
Tab detaching should not trigger after click on Master Password Dialog button

Comment 1

[Alice0775 White]

Sorry,

6. And then mouse up at top border of URLbar

6. And then mouse move within this tab and mouse up
   (i.e. this should be within detachTabThresholdY, so tab detaching does never happen)

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9eab7d33fb582d01e005b4125894d35b6d8e7690&tochange=d16bf53b358583baa538e24697f85a597e4cf41c

Regressed by:
d16bf53b3585 Sam Foster — Bug 1149500 - Delay autofill until the document is visible. r=MattN

:sfoster,
Your patch seems to cause this regression

Can you please look into this?

Comment 3

[Sam Foster [:sfoster] (he/him)]

Thanks for tracking this down. I'll take a look.

Comment 4

[Sam Foster [:sfoster] (he/him)]

Would you be able to attach a video of this? I'm trying to follow the STR, but I find (on Linux at least) that the mousedown is enough to bring the hidden tab into the foreground and focus the master password dialog. So I have no opportunity to drag the tab to detach it.

Comment 5

[Alice0775 White]

(In reply to Sam Foster [:sfoster] (he/him) from comment #4)

Would you be able to attach a video of this? I'm trying to follow the STR,
but I find (on Linux at least) that the mousedown is enough to bring the
hidden tab into the foreground and focus the master password dialog. So I
have no opportunity to drag the tab to detach it.

Steps:
Middle click on BMO link to open in background tab
mousedown on the background tab, (do not mouseup)
--- the tab is into the foreground
--- the master password dialog appears
keeping mouse down, move mouse a few pixels downward(within detachTabThresholdY)
mouseup
Click on [cancel] button
move mouse a little
--- the tab is unexpectedly detached

Screencast : https://youtu.be/73Tn7rMWx6o

FYI, And I cannot reproduce this issue on Ubuntu18.04.

Comment 6

[Alice0775 White]

Attachment: Screencast2

Another Steps:
Middle click on BMO link to open in background tab
mousedown on the background tab, (do not mouseup)
--- the tab is into the foreground
--- the master password dialog appears
keeping mouse down, move mouse a few pixels within the tab
mouseup
Click on [cancel] button
move mouse a little
--- the tab is unexpectedly detached

Comment 7

[Sam Foster [:sfoster] (he/him)]

I've been looking for ways to fix this in/around tabbrowser-tabs binding and its dragend handler, where we currently make the decision to detach the tab and there are a couple of existing early-return criteria to bail out of that. Here's some things I've found:

Before bug 1149500, the master password prompt would show immediately, so there was no real opportunity to interact with the tabstrip and get these kind of errors. Since that patch, we defer processing the forms in the document until it becomes visible, so the master password prompt isn't shown until the tab containing it becomes selected. Starting to drag a tab causes that tab to be selected, so the prompt is shown while the mouse is still down - mid-drag. It steals focus, and when you close it, the browser window starts getting mouse events again, sees the mouse has travelled far enough to constitute a drag-to-detach gesture and opens a new window for that tab.

I can listen for DOMModalDialogWillOpen and/or DOMModalDialogClosed to know if the drag session was interrupted by interacting with the modal dialog that is the master password prompt.

However, it looks like drag handling in this binding is carefully kept stateless - we may not get the initial dragstart event, we may never get the dragend event. So while I could add a marker attribute to say modaldialogwashere, its not clear when I could safely remove it.

Furthermore, you can get really weird behavior if you follow the STR and when you click to cancel the prompt, wait a few seconds, then start moving the mouse again. A few seconds after that the tab will suddenly detach itself. I guess the prompt is eating these down/up events, so the the browser window suddenly starts getting mouse move events and somewhere I've not yet found there is a threshold timeout that just ends the drag session and triggers the tabbrowser-tabs dragend handling.

All this makes me think that proper place to fix this might be in platform code rather than the binding, though in the interest of getting something upliftable I would love to find a practical workaround. Suggestions on next steps? Picking on you Gijs as the reviewer for most patches in/around this stuff.

Comment 8

[:Gijs (he/him)]

I don't know much about this, but some cursory bugzilla searching suggests Mike fixed a problem very much like this in bug 100180.

Perhaps the same fix needs applying to other dialogs?

Comment 9

[Sam Foster [:sfoster] (he/him)]

I'm going to bounce this question over to you Mike, because if Gijs doesnt know much about it, I know considerably less!

Comment 10

[:Gijs (he/him)]

I was hoping that the references to the previous patch here would be enough to help further digging.

So for reference, my next steps were:

1. add some printfs to nsGlobalWindowOuter::EnterModalState(), as that's where we're supposed to end the drag session.

2. verify that we are indeed ending any pending drag session when the master password comes up (we are)

3. wonder if we're starting a drag session after the master password comes up / disappears, or something.

4. wonder where we start the drag session. Start by looking at the windows widget (because this doesn't happen on mac/ubuntu, apparently) drag service. See StartDragSession is implemented on widget/nsBaseDragService.cpp . Add:

nsTraceRefcnt::WalkTheStack(stdout)

there to get a stack for where the drag start is coming from.

I'm gonna clear Mike's NI for now because I'm pretty sure he's busy.

Comment 11

[:Gijs (he/him)]

Stack for the drag start:

Entering modal state # this gets fired when the master password comes up
Ending drag session # this too

# The stack doesn't appear until I click the 'cancel' button on the MP dialog

#01: nsBaseDragService::InvokeDragSession (d:\mozilla-central\widget\nsBaseDragService.cpp:235)
#02: nsBaseDragService::InvokeDragSessionWithImage (d:\mozilla-central\widget\nsBaseDragService.cpp:286)
#03: mozilla::EventStateManager::DoDefaultDragStart (d:\mozilla-central\dom\events\EventStateManager.cpp:2011)
#04: mozilla::EventStateManager::GenerateDragGesture (d:\mozilla-central\dom\events\EventStateManager.cpp:1842)
#05: mozilla::EventStateManager::PreHandleEvent (d:\mozilla-central\dom\events\EventStateManager.cpp:625)
#06: mozilla::PresShell::EventHandler::DispatchEvent (d:\mozilla-central\layout\base\PresShell.cpp:7729)
#07: mozilla::PresShell::EventHandler::HandleEventWithCurrentEventInfo (d:\mozilla-central\layout\base\PresShell.cpp:7698)
#08: mozilla::PresShell::EventHandler::HandleEventWithTarget (d:\mozilla-central\layout\base\PresShell.cpp:7633)
#09: mozilla::PointerEventHandler::DispatchPointerFromMouseOrTouch (d:\mozilla-central\dom\events\PointerEventHandler.cpp:532)
#10: mozilla::PresShell::EventHandler::DispatchPrecedingPointerEvent (d:\mozilla-central\layout\base\PresShell.cpp:6874)
#11: mozilla::PresShell::EventHandler::HandleEventUsingCoordinates (d:\mozilla-central\layout\base\PresShell.cpp:6693)
#12: mozilla::PresShell::EventHandler::HandleEvent (d:\mozilla-central\layout\base\PresShell.cpp:6519)
#13: mozilla::PresShell::HandleEvent (d:\mozilla-central\layout\base\PresShell.cpp:6447)
#14: nsViewManager::DispatchEvent (d:\mozilla-central\view\nsViewManager.cpp:758)
#15: nsView::HandleEvent (d:\mozilla-central\view\nsView.cpp:1071)
#16: nsWindow::DispatchEvent (d:\mozilla-central\widget\windows\nsWindow.cpp:0)
#17: nsBaseWidget::ProcessUntransformedAPZEvent (d:\mozilla-central\widget\nsBaseWidget.cpp:974)
#18: nsBaseWidget::DispatchInputEvent (d:\mozilla-central\widget\nsBaseWidget.cpp:1112)
#19: nsWindow::DispatchMouseEvent (d:\mozilla-central\widget\windows\nsWindow.cpp:4425)
#20: nsWindow::ProcessMessage (d:\mozilla-central\widget\windows\nsWindow.cpp:5335)
#21: nsWindow::WindowProcInternal (d:\mozilla-central\widget\windows\nsWindow.cpp:4759)
#22: CallWindowProcCrashProtected (d:\mozilla-central\xpcom\base\nsCrashOnException.cpp:32)
#23: nsWindow::WindowProc (d:\mozilla-central\widget\windows\nsWindow.cpp:4712)
#24: CallWindowProcW[C:\WINDOWS\System32\user32.dll +0x1681d]
#25: DispatchMessageW[C:\WINDOWS\System32\user32.dll +0x16212]
#26: nsAppShell::ProcessNextNativeEvent (d:\mozilla-central\widget\windows\nsAppShell.cpp:540)
#27: nsBaseAppShell::OnProcessNextEvent (d:\mozilla-central\widget\nsBaseAppShell.cpp:259)
#28: nsThread::ProcessNextEvent (d:\mozilla-central\xpcom\threads\nsThread.cpp:1095)
#29: NS_ProcessNextEvent (d:\mozilla-central\xpcom\threads\nsThreadUtils.cpp:482)
#30: mozilla::ipc::MessagePump::Run (d:\mozilla-central\ipc\glue\MessagePump.cpp:88)
#31: MessageLoop::RunHandler (d:\mozilla-central\ipc\chromium\src\base\message_loop.cc:309)
#32: MessageLoop::Run (d:\mozilla-central\ipc\chromium\src\base\message_loop.cc:291)
#33: nsBaseAppShell::Run (d:\mozilla-central\widget\nsBaseAppShell.cpp:139)
#34: nsAppShell::Run (d:\mozilla-central\widget\windows\nsAppShell.cpp:411)
#35: nsAppStartup::Run (d:\mozilla-central\toolkit\components\startup\nsAppStartup.cpp:272)
#36: XREMain::XRE_mainRun (d:\mozilla-central\toolkit\xre\nsAppRunner.cpp:4589)
#37: XREMain::XRE_main (d:\mozilla-central\toolkit\xre\nsAppRunner.cpp:4727)
#38: XRE_main (d:\mozilla-central\toolkit\xre\nsAppRunner.cpp:4811)
#39: NS_internal_main (d:\mozilla-central\browser\app\nsBrowserApp.cpp:291)
#40: wmain (d:\mozilla-central\toolkit\xre\nsWindowsWMain.cpp:131)
#41: __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
#42: BaseThreadInitThunk[C:\WINDOWS\System32\KERNEL32.DLL +0x17944]
#43: RtlUserThreadStart[C:\WINDOWS\SYSTEM32\ntdll.dll +0x6ce71]

I'll also add that I noticed if I use escape to dismiss the MP dialog, no drag happens. Ditto if I use tab + spacebar to activate the [cancel] button.

Perhaps :agashlin can help with the windows event side of things, because it seems like it's windows telling us a drag happened, and presumably there's something we can do to tell win32 not to think this is a drag.

Comment 12

[Sam Foster [:sfoster] (he/him)]

(In reply to :Gijs (he/him) from comment #10)

I was hoping that the references to the previous patch here would be enough to help further digging.

So for reference, my next steps were:

1. add some printfs to nsGlobalWindowOuter::EnterModalState(), as that's where we're supposed to end the drag session.

2. verify that we are indeed ending any pending drag session when the master password comes up (we are)

3. wonder if we're starting a drag session after the master password comes up / disappears, or something.

Thanks for this. Yeah, I couldve/shouldve got at least this far myself.

4. wonder where we start the drag session. Start by looking at the windows widget (because this doesn't happen on mac/ubuntu, apparently) drag service. See StartDragSession is implemented on widget/nsBaseDragService.cpp . Add:

nsTraceRefcnt::WalkTheStack(stdout)

there to get a stack for where the drag start is coming from.

...And now I'm learning new things. I appreciate the help.
I did notice from my own logging that the dragstart/dragend appeared to happen after cancelling the prompt, but put that down to log messages getting output out of sequence due to the modal or something.

Comment 13

[Sam Foster [:sfoster] (he/him)]

Changing to P1. This issue was reported on beta so we'd like to uplift a patch if possible.

Comment 14

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

I don't know much about drag/drop, Jim can you help or redirect?

Comment 15

[Neha Kochar [:neha]]

[Tracking Requested - why for this release]:
New regression introduced in 67.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Neil Deakin might have some insight on this.

Comment 17

[Neil Deakin]

Probably EventStateManager::mGestureDownContent is set while the modal dialog occurs. If that's the case, it should be cleared by calling StopTrackingDragGesture().

Comment 18

[Sam Foster [:sfoster] (he/him)]

(In reply to Neil Deakin (away march 22 - 31) from comment #17)

Probably EventStateManager::mGestureDownContent is set while the modal dialog occurs. If that's the case, it should be cleared by calling StopTrackingDragGesture().

That sounds promising. When I break in nsGlobalWindowOuter::EnterModalState() when the password manager prompt is being shown, I can see there is an activeESM (EventStateManager), but we don't get the nsIPresShell, so that whole block is skipped.

activeESM->mGestureDownContent looks to be a valid pointer, but StopTrackingDragGesture() is protected so I can't call it from here. I'm deep inside foreign territory here, so I'm not sure how to proceed.

Also, as I understand it, although this bug is windows-specific, the mGestureDownContent can be safely cleared at this point, for all platforms?

Comment 19

[Sam Foster [:sfoster] (he/him)]

Another approach to mitigating this issue (via MattN & if I've understood correctly) would be to not defer handling the password fields in LoginManagerContent if master password is set and not logged in (which is the conditions in which this bug reproduces.) That would require knowing the logged in state from the content process, which AFAICT is not currently possible - without communicating (async) with the parent via message. This throws when called from the content process:

let tokenDB = Cc["@mozilla.org/security/pk11tokendb;1"]
                    .getService(Ci.nsIPK11TokenDB);

but maybe there's some topic we can observe from LoginManagerContent which would allow us to track this as the state changed. I'll take a look.

Comment 20

[Sam Foster [:sfoster] (he/him)]

:enn, need-info for comment #18. I think as well as any pointers you have, it would be good to get a call as to whether this has any chance of being a simple & constrained-enough fix to uplift with low risk.

Comment 21

[Neil Deakin]

The easiest option is to just make StopTrackingDragGesture public. You can call it safely, but you need to call it in the right EventStateManager. I guess that might be the active one EnterModalState gets, but I'm not sure. You might need to ask smaug about that.

A more complete option might be to move all the various state/capture clearing stuff that happens in nsGlobalWindowOuter::EnterModalState into the EventStateManager and have have that happen in one place that other callers can use.

Comment 22

[Matthew N. [:MattN]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #19)

Another approach to mitigating this issue (via MattN & if I've understood correctly) would be to not defer handling the password fields in LoginManagerContent if master password is set and not logged in (which is the conditions in which this bug reproduces.) That would require knowing the logged in state from the content process, which AFAICT is not currently possible - without communicating (async) with the parent via message. This throws when called from the content process:

let tokenDB = Cc["@mozilla.org/security/pk11tokendb;1"]
                    .getService(Ci.nsIPK11TokenDB);

but maybe there's some topic we can observe from LoginManagerContent which would allow us to track this as the state changed. I'll take a look.

Sending LoginHelper.isMasterPasswordSet from the parent would also work and not worry about login state changes. You can use Services.(cpmm|ppmm).sharedData so get the state to all new processes.

Comment 23

[Sam Foster [:sfoster] (he/him)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #22)

Sending LoginHelper.isMasterPasswordSet from the parent would also work and not worry about login state changes. You can use Services.(cpmm|ppmm).sharedData so get the state to all new processes.

Thanks for that pointer. I have a patch using that approach that seems to work. I'm getting the kinks out of the updated test then I'll get it in for review.

So, to clarify, when you have master password enabled (logged in or not) this patch will essentially revert the deferred password field handling introduced by bug 1149500. I think that's reasonable - particularly as we want to uplift to 67.

Comment 24

[Sam Foster [:sfoster] (he/him)]

Thanks for that pointer. I have a patch using that approach that seems to work. I'm getting the kinks out of the updated test then I'll get it in for review.

So, to clarify, when you have master password enabled (logged in or not) this patch will essentially revert the deferred password field handling introduced by bug 1149500. I think that's reasonable - particularly as we want to uplift to 67.

Turns out that works when MP is enabled in a previous session, but doesnt catch the case where the user enables or disables it during a session. I'm looking for a signal/moment I can use when that happens to update the Services.(cpmm|ppmm).sharedData with the value of isMasterPasswordSet so that is already known when the onDOMFormHasPassword is called in the content process. So far I'm not seeing one, it is always queried on-demand when needed and there seems to be no ready hook to use. I would rather not add a message/response from LoginManagerContent.jsm to query a value each time, in real time, that is so rarely updated.

Comment 25

[Sam Foster [:sfoster] (he/him)]

ni? for any suggestions on comment 24.

Comment 26

[Matthew N. [:MattN]]

(In reply to Sam Foster [:sfoster] (he/him) from comment #25)

ni? for any suggestions on comment 24.

I had suggested in comment 22 to "not worry about login state changes" so I don't think we should care about that edge case since this bug is already an edge case.

For testing purposes you could use https://searchfox.org/mozilla-central/rev/dd7e27f4a805e4115d0dbee70e1220b23b23c567/toolkit/components/passwordmgr/test/LoginTestUtils.jsm#28-31 which is used at https://searchfox.org/mozilla-central/rev/dd7e27f4a805e4115d0dbee70e1220b23b23c567/toolkit/components/passwordmgr/LoginManager.jsm#116-123

You would need to update your code to also happen there, ideally with one function so we're testing the same code as our normal startup.

Comment 27

[:Gijs (he/him)]

FWIW, given bug 1542353 I think it would still be nice if we could come up with a solution to the "a dialog popped up in the middle of your drag" problem, but I guess that can be a separate bug if there is a more expedient solution for the practical issue introduced by the fix in bug 1149500 here.

Comment 28

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1538460 - Only defer handling fields for login autofill when master password is not set. r?MattN

• Use Services.(ppmm|cpmm).sharedData to make isMasterPasswordSet value available to the content process
• We don't handle runtime enable/disable of MP

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f3b93e9f07c
Only defer handling fields for login autofill when master password is not set. r=MattN

Comment 31

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 6f3b93e9f07c (bug 1538460) for browser-chrome failures at toolkit/components/passwordmgr/test/browser/browser_hidden_document_autofill.js

Backout: https://hg.mozilla.org/integration/autoland/rev/d9b9b55627e5f3d31b192cdc28d02244d56e8820

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=239489274&revision=6f3b93e9f07cc79fa1ce455277658737977afcbd

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=239485263&repo=autoland&lineNumber=5343

task 2019-04-10T22:46:55.024Z] 22:46:55 INFO - TEST-PASS | toolkit/components/passwordmgr/test/browser/browser_hidden_document_autofill.js | MP Dialog should be shown when form is loaded into a hidden document -
[task 2019-04-10T22:46:55.026Z] 22:46:55 INFO - Leaving test bound test_immediate_autofill_with_masterpassword
[task 2019-04-10T22:46:55.029Z] 22:46:55 INFO - Console message: [JavaScript Error: "NS_ERROR_ABORT: User canceled master password entry" {file: "resource://gre/modules/crypto-SDR.js" line: 179}]
[task 2019-04-10T22:46:55.031Z] 22:46:55 INFO - Console message: [JavaScript Error: "NS_ERROR_ABORT: User canceled master password entry" {file: "resource://gre/modules/crypto-SDR.js" line: 179}]
[task 2019-04-10T22:46:55.035Z] 22:46:55 INFO - Console message: [JavaScript Error: "NS_ERROR_ABORT: User canceled master password entry" {file: "resource://gre/modules/crypto-SDR.js" line: 179}]
[task 2019-04-10T22:46:55.039Z] 22:46:55 INFO - Console message: [JavaScript Error: "NS_ERROR_NOT_INITIALIZED" {file: "resource://gre/modules/LoginManagerParent.jsm" line: 236}]
[task 2019-04-10T22:46:55.041Z] 22:46:55 INFO - Buffered messages finished
[task 2019-04-10T22:46:55.044Z] 22:46:55 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/passwordmgr/test/browser/browser_hidden_document_autofill.js | A promise chain failed to handle a rejection: [Exception... "Component not initialized" nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)" location: "JS frame :: resource://gre/modules/LoginManagerParent.jsm :: sendLoginDataToChild :: line 236" data: no] - stack: sendLoginDataToChild@resource://gre/modules/LoginManagerParent.jsm:236:12
[task 2019-04-10T22:46:55.046Z] 22:46:55 INFO - asyncreceiveMessage@resource://gre/modules/LoginManagerParent.jsm:81:14
[task 2019-04-10T22:46:55.048Z] 22:46:55 INFO - receiveMessage@resource:///modules/BrowserGlue.jsm:556:30
[task 2019-04-10T22:46:55.050Z] 22:46:55 INFO - MessageListener.receiveMessageinit@resource:///modules/BrowserGlue.jsm:571:19
[task 2019-04-10T22:46:55.052Z] 22:46:55 INFO - BG__beforeUIStartup@resource:///modules/BrowserGlue.jsm:1038:15
[task 2019-04-10T22:46:55.056Z] 22:46:55 INFO - BG_observe@resource:///modules/BrowserGlue.jsm:724:14
[task 2019-04-10T22:46:55.058Z] 22:46:55 INFO - Rejection date: Wed Apr 10 2019 22:46:54 GMT+0000 (Coordinated Universal Time) - false == true - JS frame :: resource://testing-common/PromiseTestUtils.jsm :: assertNoUncaughtRejections :: line 257
[task 2019-04-10T22:46:55.061Z] 22:46:55 INFO - Stack trace:
[task 2019-04-10T22:46:55.063Z] 22:46:55 INFO - resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:257
[task 2019-04-10T22:46:55.065Z] 22:46:55 INFO - chrome://mochikit/content/browser-test.js:nextTest:755
[task 2019-04-10T22:46:55.067Z] 22:46:55 INFO - chrome://mochikit/content/browser-test.js:testScope/test_finish/<:1417
[task 2019-04-10T22:46:55.069Z] 22:46:55 INFO - chrome://mochikit/content/browser-test.js:run:1354
[task 2019-04-10T22:46:55.072Z] 22:46:55 INFO - resource://gre/modules/Prompter.jsm:openModalWindow:358
[task 2019-04-10T22:46:55.074Z] 22:46:55 INFO - resource://gre/modules/Prompter.jsm:openPrompt:566
[task 2019-04-10T22:46:55.076Z] 22:46:55 INFO - resource://gre/modules/Prompter.jsm:nsIPrompt_promptPassword:783
[task 2019-04-10T22:46:55.078Z] 22:46:55 INFO - resource://gre/modules/Prompter.jsm:promptPassword:597
[task 2019-04-10T22:46:55.080Z] 22:46:55 INFO - resource://gre/modules/crypto-SDR.js:decrypt:164
[task 2019-04-10T22:46:55.082Z] 22:46:55 INFO - resource://gre/modules/storage-json.js:_decryptLogins:523
[task 2019-04-10T22:46:55.085Z] 22:46:55 INFO - resource://gre/modules/storage-json.js:searchLogins:291
[task 2019-04-10T22:46:55.087Z] 22:46:55 INFO - resource://gre/modules/LoginManager.jsm:searchLogins:410
[task 2019-04-10T22:46:55.091Z] 22:46:55 INFO - resource://gre/modules/LoginHelper.jsm:searchLoginsWithObject:182
[task 2019-04-10T22:46:55.093Z] 22:46:55 INFO - resource://gre/modules/LoginManagerParent.jsm:_searchAndDedupeLogins:53
[task 2019-04-10T22:46:55.095Z] 22:46:55 INFO - resource://gre/modules/LoginManagerParent.jsm:sendLoginDataToChild:230
[task 2019-04-10T22:46:55.097Z] 22:46:55 INFO - resource://gre/modules/LoginManagerParent.jsm:receiveMessage:81
[task 2019-04-10T22:46:55.099Z] 22:46:55 INFO - resource:///modules/BrowserGlue.jsm:receiveMessage:556
[task 2019-04-10T22:46:55.101Z] 22:46:55 INFO - resource:///modules/BrowserGlue.jsm:init:571
[task 2019-04-10T22:46:55.105Z] 22:46:55 INFO - resource:///modules/BrowserGlue.jsm:BG__beforeUIStartup:1038
[task 2019-04-10T22:46:55.107Z] 22:46:55 INFO - resource:///modules/BrowserGlue.jsm:BG_observe:724
[task 2019-04-10T22:46:55.110Z] 22:46:55 INFO - GECKO(4584) | MEMORY STAT | vsize 20974275MB | residentFast 1118MB

Comment 32

[Sam Foster [:sfoster] (he/him)]

(In reply to :Gijs (he/him) from comment #27)

FWIW, given bug 1542353 I think it would still be nice if we could come up with a solution to the "a dialog popped up in the middle of your drag" problem, but I guess that can be a separate bug if there is a more expedient solution for the practical issue introduced by the fix in bug 1149500 here.

I've opened bug 1544167 for this.

Comment 33

[Sam Foster [:sfoster] (he/him)]

I'm getting a weird error for win7-debug on my try runs. I disabled the test, but as the same error happens in multiple b-c chunks it must be related to the changes in LoginManager.jsm or LoginManagerContent.jsm.

E.g https://treeherder.mozilla.org/#/jobs?repo=try&revision=5326605eafab5c6822891a29e28658376563b471&selectedJob=240093804

"GECKO(4712) | Assertion failure: false (d3dcompiler DLL loading failed.), at z:/build/build/src/gfx/gl/GLLibraryEGL.cpp:431"

The stack trace from the logs looks like:

Assertion failure: false (d3dcompiler DLL loading failed.), at z:/build/build/src/gfx/gl/GLLibraryEGL.cpp:431
#01: mozilla::gl::GLLibraryEGL::EnsureInitialized(bool,nsTSubstring<char> * const) [gfx/gl/GLLibraryEGL.cpp:385]
#02: mozilla::gl::GLContextProviderEGL::CreateOffscreen(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const &,mozilla::gl::SurfaceCaps const &,mozilla::gl::CreateContextFlags,nsTSubstring<char> * const) [gfx/gl/GLContextProviderEGL.cpp:1131]
#03: mozilla::WebGLContext::CreateAndInitGL(bool,std::vector<mozilla::WebGLContext::FailureReason,std::allocator<mozilla::WebGLContext::FailureReason> > * const) [dom/canvas/WebGLContext.cpp:590]
#04: mozilla::WebGLContext::SetDimensions(int,int) [dom/canvas/WebGLContext.cpp:843]
#05: mozilla::dom::CanvasRenderingContextHelper::UpdateContext(JSContext *,JS::Handle<JS::Value>,mozilla::ErrorResult &) [dom/canvas/CanvasRenderingContextHelper.cpp:216]
#06: mozilla::dom::CanvasRenderingContextHelper::GetContext(JSContext *,nsTSubstring<char16_t> const &,JS::Handle<JS::Value>,mozilla::ErrorResult &) [dom/canvas/CanvasRenderingContextHelper.cpp:175]
#07: mozilla::dom::HTMLCanvasElement::GetContext(JSContext *,nsTSubstring<char16_t> const &,JS::Handle<JS::Value>,mozilla::ErrorResult &) [dom/html/HTMLCanvasElement.cpp:914]
#08: static bool mozilla::dom::HTMLCanvasElement_Binding::getContext(struct JSContext *, class JS::Handle<JSObject *>, class mozilla::dom::HTMLCanvasElement *, const class JSJitMethodCallArgs & const) [s3:gecko-generated-sources:9596d3456e6c5e07bff7d74cea7699d6a0fa8fa46dbbdd3aea05390f7cfe37043d4ff950e1ddb603305acb1aeee5c4e3b78dc396207d3f285aff9fbb3cdd970e/dom/bindings/HTMLCanvasElementBinding.cpp::289]
#09: mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy,mozilla::dom::binding_detail::ThrowExceptions>(JSContext *,unsigned int,JS::Value *) [dom/bindings/BindingUtils.cpp:3150]
#10: CallJSNative(JSContext *,bool (*)(JSContext *,unsigned int,JS::Value *),JS::CallArgs const &) [js/src/vm/Interpreter.cpp:442]
#11: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:534]
#12: static bool InternalCall(struct JSContext *, const class js::AnyInvokeArgs & const) [js/src/vm/Interpreter.cpp:589]
#13: static bool Interpret(struct JSContext *, class js::RunState & const) [js/src/vm/Interpreter.cpp:0]
#14: js::RunScript(JSContext *,js::RunState &) [js/src/vm/Interpreter.cpp:422]
#15: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:562]
#16: static bool InternalCall(struct JSContext *, const class js::AnyInvokeArgs & const) [js/src/vm/Interpreter.cpp:589]
#17: js::jit::DoCallFallback(JSContext *,js::jit::BaselineFrame *,js::jit::ICCall_Fallback *,unsigned int,JS::Value *,JS::MutableHandle<JS::Value>) [js/src/jit/BaselineIC.cpp:3879]
#18: ??? (???:???)"}
#19: ??? (???:???)"}
#20: ??? (???:???)"}
#21: js::jit::EnterBaselineAtBranch(JSContext *,js::InterpreterFrame *,unsigned char *) [js/src/jit/BaselineJIT.cpp:189]
#22: static bool Interpret(struct JSContext *, class js::RunState & const) [js/src/vm/Interpreter.cpp:1979]
#23: js::RunScript(JSContext *,js::RunState &) [js/src/vm/Interpreter.cpp:422]
#24: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:562]
#25: static bool InternalCall(struct JSContext *, const class js::AnyInvokeArgs & const) [js/src/vm/Interpreter.cpp:589]
#26: js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:605]
#27: JS::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,JS::HandleValueArray const &,JS::MutableHandle<JS::Value>) [js/src/jsapi.cpp:2620]
#28: mozilla::dom::EventListener::HandleEvent(JSContext *,JS::Handle<JS::Value>,mozilla::dom::Event &,mozilla::ErrorResult &) [s3:gecko-generated-sources:990b60ace5fb196878f08a6390ee6e0de154cbf84ad9e3dbb3c227ee7976a03523839aef3af495bdf29a17e87b825171ce3823c71b26dafbb1c81ba07118c635/dom/bindings/EventListenerBinding.cpp::52]
#29: mozilla::dom::EventListener::HandleEvent<mozilla::dom::EventTarget *>(mozilla::dom::EventTarget * const &,mozilla::dom::Event &,mozilla::ErrorResult &,char const *,mozilla::dom::CallbackObject::ExceptionHandling,JS::Realm *) [s3:gecko-generated-sources:e25e515f217c282a68188c9505f6f2a75502c46aab5c9cb3a06dcc136a4aba695d695aa47bd4d26329a8893d8751453a184ab825018035a3fae0d78dcecc6aa7/dist/include/mozilla/dom/EventListenerBinding.h::66]
#30: mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener *,mozilla::dom::Event *,mozilla::dom::EventTarget *) [dom/events/EventListenerManager.cpp:1041]
#31: mozilla::EventListenerManager::HandleEventInternal(nsPresContext *,mozilla::WidgetEvent *,mozilla::dom::Event * *,mozilla::dom::EventTarget *,nsEventStatus *,bool) [dom/events/EventListenerManager.cpp:1242]
#32: mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor &,mozilla::ELMCreationDetector &) [dom/events/EventDispatcher.cpp:352]
#33: mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem> &,mozilla::EventChainPostVisitor &,mozilla::EventDispatchingCallback *,mozilla::ELMCreationDetector &) [dom/events/EventDispatcher.cpp:553]
#34: mozilla::EventDispatcher::Dispatch(nsISupports *,nsPresContext *,mozilla::WidgetEvent *,mozilla::dom::Event *,nsEventStatus *,mozilla::EventDispatchingCallback *,nsTArray<mozilla::dom::EventTarget *> *) [dom/events/EventDispatcher.cpp:1046]
#35: nsDocumentViewer::LoadComplete(nsresult) [layout/base/nsDocumentViewer.cpp:1098]
#36: nsDocShell::EndPageLoad(nsIWebProgress *,nsIChannel *,nsresult) [docshell/base/nsDocShell.cpp:6589]
#37: nsDocShell::OnStateChange(nsIWebProgress *,nsIRequest *,unsigned int,nsresult) [docshell/base/nsDocShell.cpp:6390]
#38: nsDocLoader::DoFireOnStateChange(nsIWebProgress * const,nsIRequest * const,int &,nsresult) [uriloader/base/nsDocLoader.cpp:1313]
#39: nsDocLoader::doStopDocumentLoad(nsIRequest *,nsresult) [uriloader/base/nsDocLoader.cpp:871]
#40: nsDocLoader::DocLoaderIsEmpty(bool) [uriloader/base/nsDocLoader.cpp:700]
#41: nsDocLoader::OnStopRequest(nsIRequest *,nsresult) [uriloader/base/nsDocLoader.cpp:599]
#42: mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest *,nsISupports *,nsresult) [netwerk/base/nsLoadGroup.cpp:568]
#43: mozilla::dom::Document::DoUnblockOnload() [dom/base/Document.cpp:7851]
#44: mozilla::dom::Document::UnblockOnload(bool) [dom/base/Document.cpp:7781]
#45: mozilla::dom::Document::DispatchContentLoadedEvents() [dom/base/Document.cpp:4910]
#46: nsresult mozilla::detail::RunnableMethodImpl<mozilla::dom::Document *,void (mozilla::dom::Document::*)() __attribute__((thiscall)),1,mozilla::RunnableKind::Standard>::Run() [xpcom/threads/nsThreadUtils.h:1177]
#47: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:1180]
#48: NS_ProcessNextEvent(nsIThread *,bool) [xpcom/threads/nsThreadUtils.cpp:486]
#49: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:88]
#50: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:315]
#51: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:309]
#52: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:291]
#53: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:139]
#54: nsAppShell::Run() [widget/windows/nsAppShell.cpp:412]
#55: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:271]
#56: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4572]
#57: XREMain::XRE_main(int,char * * const,mozilla::BootstrapConfig const &) [toolkit/xre/nsAppRunner.cpp:4710]
#58: XRE_main(int,char * * const,mozilla::BootstrapConfig const &) [toolkit/xre/nsAppRunner.cpp:4791]
#59: mozilla::BootstrapImpl::XRE_main(int,char * * const,mozilla::BootstrapConfig const &) [toolkit/xre/Bootstrap.cpp:45]
#60: NS_internal_main(int,char * *,char * *) [browser/app/nsBrowserApp.cpp:291]
#61: wmain [toolkit/xre/nsWindowsWMain.cpp:131]
#62: static int __scrt_common_main_seh() [f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:288]
#63: kernel32.dll + 0x53c45
#64: ntdll.dll + 0x637f5
#65: ntdll.dll + 0x637c8

..None of which makes much sense to me.

• Why win7-debug only, when win7-opt (and win10-debug and win10-opt) pass just fine?
• Why would it pass for some bc chunks and not others? (I guess some end up loading these components and other don't).
• What has a webgl context got to do with loading login-related logic?

Comment 34

[Matthew N. [:MattN]]

It looks like the reason the patch was backed out was for 1 bc4 Windows 10 failure on 3 consecutive pushes surrounded by passes on the other pushes:
https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=windows%2C10%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Ce10s%2Ctest-windows10-64%2Fdebug-mochitest-browser-chrome-e10s-4%2Cm-e10s%28bc4%29&tochange=710344cf9b5a442b0d4de8925ac63e4ea2fbd26b&fromchange=6f3b93e9f07cc79fa1ce455277658737977afcbd

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f6e8916a498
Only defer handling fields for login autofill when master password is not set. r=MattN

Comment 36

[Cristina Coroiu [:ccoroiu]]

Here is the link with bc perma failing on OS X 10.10 shippable opt, that is why it was backed out:
https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=os%2Cx%2C10.10%2Cshippable%2Copt%2Cmochitests%2Cwith%2Ce10s%2Ctest-macosx64-shippable%2Fopt-mochitest-browser-chrome-e10s-7%2Cm-e10s%28bc7%29&tochange=7f0e024013704d89e01b6f8103fbfd6ef5f0107b&fromchange=10b758c2be41d7fce59d229cfdc1e7091ef0165b

Comment 37

[Sam Foster [:sfoster] (he/him)]

To summarize the behavior we expect with this patch:

Without master password set (default):

When you open new background tabs, no attempt is made to autofill logins forms until each tab is made visible. This is apparent if you enable the signon.debug preference and check the logs. Also/instead you can manually verify this by:

1. in a new profile, Open https://bugzilla.mozilla.org, and right-click to open the sign in page in a new tab.
2. Stay in the first tab and sign in to bugzilla in this tab, accepting the prompt to save the password.
3. Drag the 2nd tab to the position before the first tab.
   Expected: As you mousedown on the tab to drag it, it becomes selected. The drag/drop should complete without interruption and it should autofill the sign in form with the new login which didn't exist when this tab was initially opened.

With master password set:

When you open new background tabs, when MP is enabled, we will attempt to autofill logins forms in background tabs as necessary, which causes the master password prompt to show as soon as the tab is loaded. This is the previous behavior from before bug 1149500 and prevents the tab drag/drop issue described in this bug and bug 1539091. You can verify this by:

1. In a new profile, enable and set a master password
2. Login to bugzilla and accept the prompt to save the password.
3. Clear cookies & site data via preferences/options and restart the browser so you are no longer logged in to bugzilla and master password
4. Load bugzilla in the first tab, and right-click to open the sign in page in a new background tab.
   Expected: As the sign in page loads in the 2nd (background tab), the master password prompt will immediately show, and block further interaction with that browser window. Once logged in to master password, the sign in form in that 2nd tab will be autofilled with your login info. You can select it to confirm, or you can mousedown on the 2nd tab to drag it and cause it to become selected as before. The drag/drop should complete without interruption

Comment 38

[Sam Foster [:sfoster] (he/him)]

(In reply to Cristina Coroiu [:ccoroiu] from comment #36)

Here is the link with bc perma failing on OS X 10.10 shippable opt, that is why it was backed out:
https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=os%2Cx%2C10.10%2Cshippable%2Copt%2Cmochitests%2Cwith%2Ce10s%2Ctest-macosx64-shippable%2Fopt-mochitest-browser-chrome-e10s-7%2Cm-e10s%28bc7%29&tochange=7f0e024013704d89e01b6f8103fbfd6ef5f0107b&fromchange=10b758c2be41d7fce59d229cfdc1e7091ef0165b

That seems to show bug 1185000 getting backed out and fixing the bc perma-fail on bc7 for OS X 10.10 shippable opt?

Comment 39

[Cristina Coroiu [:ccoroiu]]

Sorry for the link, it was another backout.

This bug was backed out for browser-chrome, on the next push there are many bc's failing on browser_hidden_document_autofill.js:
https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=browser-chrome&tochange=4d0388ce7f1addfbea33f535d2ffc8719b1e42f5&fromchange=6f3b93e9f07cc79fa1ce455277658737977afcbd&selectedJob=239466863

Here are all the browser-chrome between this push and backout:
https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=browser-chrome&tochange=710344cf9b5a442b0d4de8925ac63e4ea2fbd26b&fromchange=c15b9e3615f7f19caeb490599192eb373f9d6480&selectedJob=239512720

Comment 40

[Sam Foster [:sfoster] (he/him)]

I had re-queued attachment 9057292 [details] for landing after comment 34 (and before comment 39.) It looks like that will need backing out again - apologies in advance. I'm going to investigate a fix/workaround for this unhandled promise rejection (Component not initialized) issue.

Comment 41

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/2f6e8916a498

Comment 42

[Sam Foster [:sfoster] (he/him)]

:adrian_sv, would you be able expedite verifying this, I'd like to get this uplifted this week if possible. I provided some pointers in comment #37

Comment 43

[Daniel Bodea [:danibodea]]

I have reproduced this issue in Nightly v68.0a1 from 2019-03-23 and I have verified the fix with Nightly v68.0a1 from 2019-04-17.
I confirm the behavior detailed in comment 37. Leaving the qe-verify+ tag and bug status for uplift verification. Thanks!

Comment 44

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 9057292 [details]
Bug 1538460 - Only defer handling fields for login autofill when master password is not set. r?MattN

Beta/Release Uplift Approval Request

• User impact if declined: With master password set, drag/drop tab strip interactions can lead to unexpected behavior - tabs can get detached into a new window, or rearranging of tabs being interrupted by the master password dialog.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment #37 for details steps and expectations.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small, front-end only patch confined to form autofill (password manager component). The change is to roll back some of the behavior changed by bug 1149500 so users with a master password set will get the same behavior they are used to.
• String changes made/needed: None

Comment 45

[Pascal Chevrel:pascalc (PTO until April 26)]

FYI, I'll take this patch into beta once it's been verified in Nightly.

Comment 46

[Adrian Florinescu [:aflorinescu]]

(In reply to Pascal Chevrel:pascalc from comment #45)

FYI, I'll take this patch into beta once it's been verified in Nightly.

See comment43, it has already been verified on Nightly. :)

Comment 47

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9057292 [details]
Bug 1538460 - Only defer handling fields for login autofill when master password is not set. r?MattN

I missed comment #43, sorry :) Uplift approved for 67 beta 13, thanks!

Comment 48

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d2668e1efcfd

Comment 49

[Daniel Bodea [:danibodea]]

I have also verified that the fix in Firefox Beta v67.0b13. The behavior detailed in comment 37 is respected.
Uplift successful. Thank you.