1338396 - Permaorange in browser_context_menu_autocomplete_interaction.js | Autocomplete shouldn't appear - false == true when Gecko 53 merges to beta 2017-03-06

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[Phil Ringnalda (:philor)]

May or may not be related to the intermittent bug 1337772, I wouldn't expect that we'll know until someone figures out the cause of one or both.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99f7a3279685024128414d87c169fb51569732aa&group_state=expanded&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=76013352 is the current aurora as though it had been merged to beta, with permaorange in browser_context_menu_autocomplete_interaction.js

[Tracking Requested - why for this release]: permaorange, closed trees, sadness.

Comment 1

[Matthew N. [:MattN]]

Hmm… I was depending on some event.timeStamp recent changes and maybe they were behind a build flag… Yep, I see bug 1256562 uses a build flag:

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

This will be easy to fix if I switch to Date.now()

Comment 2

[Matthew N. [:MattN]]

Attachment: Bug 1338396 - LoginManagerContent: Use Date.now() instead of event.timeStamp until high-res timestamps are shipping.

Review commit: https://reviewboard.mozilla.org/r/112082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112082/

Comment 3

[Matthew N. [:MattN]]

Feel free to autoland if it gets r+.

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8836736 [details]
Bug 1338396 - LoginManagerContent: Use Date.now() instead of event.timeStamp until high-res timestamps are shipping.

https://reviewboard.mozilla.org/r/112082/#review113340

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e830c124ad16
LoginManagerContent: Use Date.now() instead of event.timeStamp until high-res timestamps are shipping. r=johannh

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8836736 [details]
Bug 1338396 - LoginManagerContent: Use Date.now() instead of event.timeStamp until high-res timestamps are shipping.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1330111
[User impact if declined]: The interaction between the context menu and username autocomplete may be bad.
[Is this code covered by automated tests?]: Yes, it's what noticed the bug
[Has the fix been verified in Nightly?]: Not yet but this same fix was already used on beta.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No, trivial change
[Why is the change risky/not risky?]: Trivial change, only potential for a very slight performance impact of getting the current time with Date.now() but since it's only for contextmenu events on username fields it will be so rare to not be noticeable.
[String changes made/needed]: None

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 8

[Gerry Chang [:gchang]]

Comment on attachment 8836736 [details]
Bug 1338396 - LoginManagerContent: Use Date.now() instead of event.timeStamp until high-res timestamps are shipping.

Fix a permaorange issue related to context menu and autocomplete. Aurora53+.

Comment 9

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2476b831a480

Comment 10

[Fischer [:Fischer]]

Hi Matt,
Just a drive-by. Bug 1026804 is about to turn on dom.event.highrestimestamp.enabled by default.
So I guess it's time to do
  `gLastContextMenuEventTimeStamp = event.timeStamp`[1] and
  `let timestamp = event.timeStamp`[2]

Should we open a follow-up bug to do that?
p.s If you have no other concern, I'd be happy to help on this.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#135
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerContent.jsm#586

Comment 11

[Matthew N. [:MattN]]

We can but I don't think it's super important