<settings-languages> initialization can execute after the element has been detached. |
|||||
Issue descriptionThis problem is not showing up in ToT, because of accidental fortunate timing, but it starts becoming a problem when https://codereview.chromium.org/2738773004/ which changes setTimeout() to actually respect the 0ms value. Adding logs in the test clearly shows that preferredLanguagesPrefChanged_ listener fires after the element has been detached from the DOM. The error manifests as an assertion error thrown from [2]. Note that the test fails flakily, but it is easy to reproduce (I've always seen it fail at least 1 out of 4 attempts), when use_vulcanize=true flag is used. [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/languages_page/languages.js?l=222 [2] https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/languages_page/languages.js?l=357
,
Mar 28 2017
@michael: The culprit seems to be at https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/languages_page/languages.js?l=198, where inside the 'created' lifecycle method, we schedule an async task by waiting on a promise. The callback of the promise might executed after the element has been detached.
,
Mar 28 2017
Candidate fix at https://codereview.chromium.org/2779873003.
,
Mar 28 2017
,
Mar 28 2017
Cool, good thing we have that CrSettingsPrefs.isInitialized assert then :-) Looks like you've identified the problem and the fix. I can see if a version that still starts the API requests in created() will work, if you like, but I don't want to duplicate work.
,
Mar 28 2017
dpapad, have you reproduced this with use_vulcanize = false at all? I wonder if issue 692356 was related to this, but the logs for that are gone and the issue itself has no details.
,
Mar 28 2017
No, have not been able to reproduce with use_vulcanize=false, but I was also not running the CrSettingsLanguagesTest.Languages tests when I wrote the description of this bug. It is possible that the same issue was causing the flakiness, if CrSettingsLanguagesTest.Languages ever remove a <settings-languages> element from the DOM. Regarding experimenting with a version that still does most of the work in created(), feel free to do so, that would be helpful (I won't do it to avoid duplicated work). Let me know if it works, and we can land your fix instead of the proposed one above.
,
Mar 29 2017
,
Mar 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1696c7fdb2d074501acc0b234874b5c2fab32082 commit 1696c7fdb2d074501acc0b234874b5c2fab32082 Author: dpapad <dpapad@chromium.org> Date: Wed Mar 29 18:01:35 2017 MD Settings: Prevent <settings-languages> running init code after is detached. BUG= 706078 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2779873003 Cr-Commit-Position: refs/heads/master@{#460453} [modify] https://crrev.com/1696c7fdb2d074501acc0b234874b5c2fab32082/chrome/browser/resources/settings/languages_page/languages.js
,
Mar 29 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by dpa...@chromium.org
, Mar 28 2017