1846828 - Fix race conditions and edge cases for offering translations

Status: RESOLVED FIXED

(Firefox :: Translations, defect, P2)

Description

[Greg Tatum [:gregtatum] (REO Fx132)]

We're still getting some errant offerings of popups and weird behavior. Erik and I did a read through the code and found some smaller issues that seemed racy or problematic.

Comment 1

[Greg Tatum [:gregtatum] (REO Fx132)]

Attachment: Bug 1846828 - Guard against multiple popups opening and being racy; r?nordzilla!

Depends on D185210

Comment 2

[Greg Tatum [:gregtatum] (REO Fx132)]

Attachment: Bug 1846828 - Add more logs to show the decision process for offering a translation; r?nordzilla!

Depends on D185211

Comment 3

[Greg Tatum [:gregtatum] (REO Fx132)]

Attachment: Bug 1846828 - Ensure only valid translations will be offered; r?nordzilla!

Depends on D185212

Comment 4

[Greg Tatum [:gregtatum] (REO Fx132)]

Attachment: Bug 1846828 - Drive by fix an issue with the console not being ready; r?nordzilla

This is being triggered by a later patch that is adding new logging. I
believe this is all just because of initialization order.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d909d4b4de02
Drive by fix an issue with the console not being ready; r=nordzilla
https://hg.mozilla.org/integration/autoland/rev/42eade304753
Guard against multiple popups opening and being racy; r=nordzilla
https://hg.mozilla.org/integration/autoland/rev/a0cd72ec2666
Add more logs to show the decision process for offering a translation; r=nordzilla
https://hg.mozilla.org/integration/autoland/rev/7c3b8868096b
Ensure only valid translations will be offered; r=nordzilla

Comment 6

[Iulian Moraru]

Backed out for causing bc failures on browser_fullscreen-bug-1798219.js related to TranslationsParent.sys.mjs.

• Push with failures

• Failure log

• Backout link

[task 2023-08-03T15:36:18.014Z] 15:36:18     INFO - TEST-PASS | dom/base/test/fullscreen/browser_fullscreen-bug-1798219.js | The chrome document should not be in fullscreen - 
[task 2023-08-03T15:36:18.015Z] 15:36:18     INFO - Leaving test bound 
[task 2023-08-03T15:36:18.015Z] 15:36:18     INFO - Buffered messages finished
[task 2023-08-03T15:36:18.018Z] 15:36:18     INFO - TEST-UNEXPECTED-FAIL | dom/base/test/fullscreen/browser_fullscreen-bug-1798219.js | A promise chain failed to handle a rejection: this.browsingContext.currentWindowGlobal is null - stack: maybeOfferTranslations@resource://gre/actors/TranslationsParent.sys.mjs:315:29
[task 2023-08-03T15:36:18.024Z] 15:36:18     INFO - receiveMessage@resource://gre/actors/TranslationsParent.sys.mjs:672:16
[task 2023-08-03T15:36:18.025Z] 15:36:18     INFO - async*handleEvent@resource://gre/actors/TranslationsChild.sys.mjs:33:14
[task 2023-08-03T15:36:18.025Z] 15:36:18     INFO - Rejection date: Thu Aug 03 2023 15:36:17 GMT+0000 (Coordinated Universal Time) - false == true - {"filename":"resource://testing-common/PromiseTestUtils.sys.mjs","name":"assertNoUncaughtRejections","sourceId":566,"lineNumber":266,"columnNumber":14,"sourceLine":"","asyncCause":null,"asyncCaller":null,"caller":{"filename":"chrome://mochikit/content/browser-test.js","name":"nextTest","sourceId":530,"lineNumber":766,"columnNumber":29,"sourceLine":"","asyncCause":null,"asyncCaller":{"filename":"chrome://mochikit/content/browser-test.js","name":"testScope/test_finish/<","sourceId":530,"lineNumber":1794,"columnNumber":25,"sourceLine":"","asyncCause":null,"asyncCaller":null,"caller":{"filename":"chrome://mochikit/content/browser-test.js","name":"run","sourceId":530,"lineNumber":1714,"columnNumber":9,"sourceLine":"","asyncCause":null,"asyncCaller":null,"caller":null,"formattedStack":"run@chrome://mochikit/content/browser-test.js:1714:9\n","nativeSavedFrame":{}},"formattedStack":"async*testScope/test_finish/<@chrome://mochikit/content/browser-test.js:1794:25\nrun@chrome://mochikit/content/browser-test.js:1714:9\n","nativeSavedFrame":{}},"caller":null,"formattedStack":"nextTest@chrome://mochikit/content/browser-test.js:766:29\nasync*testScope/test_finish/<@chrome://mochikit/content/browser-test.js:1794:25\nrun@chrome://mochikit/content/browser-test.js:1714:9\n","nativeSavedFrame":{}},"formattedStack":"assertNoUncaughtRejections@resource://testing-common/PromiseTestUtils.sys.mjs:266:14\nnextTest@chrome://mochikit/content/browser-test.js:766:29\nasync*testScope/test_finish/<@chrome://mochikit/content/browser-test.js:1794:25\nrun@chrome://mochikit/content/browser-test.js:1714:9\n","nativeSavedFrame":{}}
[task 2023-08-03T15:36:18.025Z] 15:36:18     INFO - Stack trace:
[task 2023-08-03T15:36:18.026Z] 15:36:18     INFO - resource://testing-common/PromiseTestUtils.sys.mjs:assertNoUncaughtRejections:266
[task 2023-08-03T15:36:18.026Z] 15:36:18     INFO - chrome://mochikit/content/browser-test.js:nextTest:766
[task 2023-08-03T15:36:18.027Z] 15:36:18     INFO - chrome://mochikit/content/browser-test.js:testScope/test_finish/<:1794
[task 2023-08-03T15:36:18.027Z] 15:36:18     INFO - chrome://mochikit/content/browser-test.js:run:1714
[task 2023-08-03T15:36:18.028Z] 15:36:18     INFO - Console message: [JavaScript Warning: "Script terminated by timeout at:
[task 2023-08-03T15:36:18.029Z] 15:36:18     INFO - @http://mochi.test:8888/browser/dom/base/test/fullscreen/file_fullscreen-bug-1798219-2.html line 7 > injectedScript:9:17
[task 2023-08-03T15:36:18.029Z] 15:36:18     INFO - " {file: "http://mochi.test:8888/browser/dom/base/test/fullscreen/file_fullscreen-bug-1798219-2.html line 7 > injectedScript" line: 9}]
[task 2023-08-03T15:36:18.030Z] 15:36:18     INFO - Console message: [JavaScript Error: "TypeError: Fullscreen request aborted"]
[task 2023-08-03T15:36:18.031Z] 15:36:18     INFO - GECKO(1592) | JavaScript error: resource://gre/actors/TranslationsParent.sys.mjs, line 315: TypeError: this.browsingContext.currentWindowGlobal is null
[task 2023-08-03T15:36:18.033Z] 15:36:18     INFO - Console message: [JavaScript Error: "TypeError: this.browsingContext.currentWindowGlobal is null" {file: "resource://gre/actors/TranslationsParent.sys.mjs" line: 315}]
[task 2023-08-03T15:36:18.033Z] 15:36:18     INFO - GECKO(1592) | MEMORY STAT | vsize 20983644MB | residentFast 1350MB
[task 2023-08-03T15:36:18.034Z] 15:36:18     INFO - TEST-OK | dom/base/test/fullscreen/browser_fullscreen-bug-1798219.js | took 5070ms
[task 2023-08-03T15:36:18.034Z] 15:36:18     INFO - GECKO(1592) | -----------------------------------------------------
[task 2023-08-03T15:36:18.037Z] 15:36:18     INFO - GECKO(1592) | Suppressions used:
[task 2023-08-03T15:36:18.038Z] 15:36:18     INFO - GECKO(1592) |   count      bytes template
[task 2023-08-03T15:36:18.038Z] 15:36:18     INFO - GECKO(1592) |       2        288 libfontconfig.so
[task 2023-08-03T15:36:18.039Z] 15:36:18     INFO - GECKO(1592) | -----------------------------------------------------
[task 2023-08-03T15:36:18.040Z] 15:36:18     INFO - checking window state
[task 2023-08-03T15:36:18.040Z] 15:36:18     INFO - GECKO(1592) | -----------------------------------------------------
[task 2023-08-03T15:36:18.041Z] 15:36:18     INFO - GECKO(1592) | Suppressions used:
[task 2023-08-03T15:36:18.044Z] 15:36:18     INFO - GECKO(1592) |   count      bytes template
[task 2023-08-03T15:36:18.045Z] 15:36:18     INFO - GECKO(1592) |       2        288 libfontconfig.so
[task 2023-08-03T15:36:18.046Z] 15:36:18     INFO - GECKO(1592) | -----------------------------------------------------
[task 2023-08-03T15:36:18.068Z] 15:36:18     INFO - TEST-START | dom/base/test/fullscreen/browser_fullscreen-contextmenu-esc.js

Comment 7

[Phabricator Automation]

Comment on attachment 9347164 [details]
Bug 1846828 - Drive by fix an issue with the console not being ready; r?nordzilla

Revision D185210 was moved to bug 1845518. Setting attachment 9347164 [details] to obsolete.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e29f1b9d0ad
Drive by fix an issue with the console not being ready; r=nordzilla
https://hg.mozilla.org/integration/autoland/rev/22396aa00df4
Guard against multiple popups opening and being racy; r=nordzilla
https://hg.mozilla.org/integration/autoland/rev/da0b9141fd86
Add more logs to show the decision process for offering a translation; r=nordzilla
https://hg.mozilla.org/integration/autoland/rev/f7b54937d508
Ensure only valid translations will be offered; r=nordzilla

Comment 9

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/5e29f1b9d0ad
https://hg.mozilla.org/mozilla-central/rev/22396aa00df4
https://hg.mozilla.org/mozilla-central/rev/da0b9141fd86
https://hg.mozilla.org/mozilla-central/rev/f7b54937d508

Comment 10

[Greg Tatum [:gregtatum] (REO Fx132)]

Comment on attachment 9347028 [details]
Bug 1846828 - Guard against multiple popups opening and being racy; r?nordzilla!

Beta/Release Uplift Approval Request

• User impact if declined: The user may be presented with a translation dialog box for an invalid translation, for instance, translating from their language to a blank language. The dialog may flash up for reasons that don't make sense.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: This is fixing intermittent edge cases without clear 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):
• String changes made/needed:
• Is Android affected?: Yes

Comment 11

[Greg Tatum [:gregtatum] (REO Fx132)]

I missed: "Why is the change risky/not risky? (and alternatives if risky):"

I would say the risk is fairly low as we are covered by automated tests, and this changes the behavior for when we trigger the popup to make it more correct.

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9347028 [details]
Bug 1846828 - Guard against multiple popups opening and being racy; r?nordzilla!

Approved for 117.0b5

Comment 13

[Donal Meehan [:dmeehan]]

Comment on attachment 9347029 [details]
Bug 1846828 - Add more logs to show the decision process for offering a translation; r?nordzilla!

Approved for 117.0b5

Comment 14

[Donal Meehan [:dmeehan]]

Comment on attachment 9347030 [details]
Bug 1846828 - Ensure only valid translations will be offered; r?nordzilla!

Approved for 117.0b5

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/906878a23bd3
https://hg.mozilla.org/releases/mozilla-beta/rev/4474585a82e4
https://hg.mozilla.org/releases/mozilla-beta/rev/151e1cc44c36
https://hg.mozilla.org/releases/mozilla-beta/rev/9119617bfdfc

Comment 16

[Donal Meehan [:dmeehan]]

Comment on attachment 9347164 [details]
Bug 1846828 - Drive by fix an issue with the console not being ready; r?nordzilla

Approved for 117.0b5

Comment 18

[Andra Esanu (needinfo me)]

(In reply to Pulsebot from comment #5)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d909d4b4de02
Drive by fix an issue with the console not being ready; r=nordzilla
https://hg.mozilla.org/integration/autoland/rev/42eade304753
Guard against multiple popups opening and being racy; r=nordzilla
https://hg.mozilla.org/integration/autoland/rev/a0cd72ec2666
Add more logs to show the decision process for offering a translation;
r=nordzilla
https://hg.mozilla.org/integration/autoland/rev/7c3b8868096b
Ensure only valid translations will be offered; r=nordzilla

== Change summary for alert #39251 (as of Thu, 10 Aug 2023 05:14:28 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
3%	ebay ContentfulSpeedIndex	windows10-64-shippable-qr	cold fission webrender	1,253.93 -> 1,221.97
2%	ebay SpeedIndex	windows10-64-shippable-qr	cold fission webrender	1,297.95 -> 1,271.13
2%	ebay SpeedIndex	windows10-64-shippable-qr	cold fission webrender	1,306.52 -> 1,282.12

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=39251