Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2042283002:
Postpone JS calls from CoreOobeHandler until the JS side gets initialized (Closed)
|
Created:
4 years, 6 months ago by emaxx Modified:
4 years, 6 months ago Reviewers:
xiyuan CC:
chromium-reviews, dzhioev+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPostpone JS calls from CoreOobeHandler until the JS side gets initialized
This fixes a race between loading JavaScript code and calling methods from it.
The race was caused by CoreOobeHandler class whose methods (like ReloadContent)
called JavaScript methods directly, while there was a possibility that JavaScript
files defining them were not loaded yet.
This was observed, for example, when running tests inherited from
LoginManagerTest, as the form of the following errors printed into console:
* [ERROR:CONSOLE(1)] "Uncaught ReferenceError: cr is not defined", source: (1)
* [ERROR:CONSOLE(1)] "Uncaught TypeError: Cannot read property 'reloadContent'
of undefined", source: (1)
This CL adds a new condition that doesn't allow to call JS methods until
'screenStateInitialize' message is received from JS side. Any attempts to call
JS method too early are deferred and executed when the 'screenStateInitialize'
is finally received.
BUG=614805
TEST=manual: run login-related tests and check that no JavaScript errors appear in the console.
Committed: https://crrev.com/e873b56e4b1cec5bdb3ccf6e8b7ccd5e97d94c37
Cr-Commit-Position: refs/heads/master@{#398342}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fix review nits, add comment near the tricky code #Patch Set 3 : Add forgotten include #
Messages
Total messages: 16 (7 generated)
Description was changed from ========== Postpone JS calls from CoreOobeHandler until the JS side gets initialized BUG=614805 ========== to ========== Postpone JS calls from CoreOobeHandler until the JS side gets initialized This fixes a race between loading JavaScript code and calling methods from it. The race was caused by CoreOobeHandler class whose methods (like ReloadContent) called JavaScript methods directly, while there was a possibility that JavaScript files defining them were not loaded yet. This CL adds a new condition that doesn't allow to call JS methods until 'screenStateInitialize' message is received from JS side. Any attempts to call JS method too early are deferred and executed when the 'screenStateInitialize' is finally received. BUG=614805 ==========
emaxx@chromium.org changed reviewers: + xiyuan@chromium.org
Hi Xiyuan, PTAL.
Description was changed from ========== Postpone JS calls from CoreOobeHandler until the JS side gets initialized This fixes a race between loading JavaScript code and calling methods from it. The race was caused by CoreOobeHandler class whose methods (like ReloadContent) called JavaScript methods directly, while there was a possibility that JavaScript files defining them were not loaded yet. This CL adds a new condition that doesn't allow to call JS methods until 'screenStateInitialize' message is received from JS side. Any attempts to call JS method too early are deferred and executed when the 'screenStateInitialize' is finally received. BUG=614805 ========== to ========== Postpone JS calls from CoreOobeHandler until the JS side gets initialized This fixes a race between loading JavaScript code and calling methods from it. The race was caused by CoreOobeHandler class whose methods (like ReloadContent) called JavaScript methods directly, while there was a possibility that JavaScript files defining them were not loaded yet. This was observed, for example, when running tests inherited from LoginManagerTest, as the form of the following errors printed into console: * [ERROR:CONSOLE(1)] "Uncaught ReferenceError: cr is not defined", source: (1) * [ERROR:CONSOLE(1)] "Uncaught TypeError: Cannot read property 'reloadContent' of undefined", source: (1) This CL adds a new condition that doesn't allow to call JS methods until 'screenStateInitialize' message is received from JS side. Any attempts to call JS method too early are deferred and executed when the 'screenStateInitialize' is finally received. BUG=614805 TEST=manual: run login-related tests and check that no JavaScript errors appear in the console. ==========
Cool. https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right): https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:176: &CoreOobeHandler::ExecuteDeferredJSCall< Is it possible to base::Bind to CallJS directly instead of having ExecuteDeferredJSCall as middle man? https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:184: for (const base::Callback<void()>& deferred_js_call : deferred_js_calls_) nit: for (const auto& deferred_js_call : deferred_js_calls_) https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:186: } deferred_js_calls_.clear(); after we are done. https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h (right): https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h:167: std::vector<base::Callback<void()>> deferred_js_calls_; nit: base::Callback<void()> -> base::Closure
https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right): https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:176: &CoreOobeHandler::ExecuteDeferredJSCall< On 2016/06/07 16:43:03, xiyuan wrote: > Is it possible to base::Bind to CallJS directly instead of having > ExecuteDeferredJSCall as middle man? I don't see an easy way, because the arguments passed to CallJSOrDefer are const-references, i.e. pointers or references to them cannot be stored. As base::Value is non-copyable and non-movable, then I had to use smart pointer for storing it and passing. But CallJS cannot be called with a smart pointer, it accepts const-references to the actual values. As another thought, lambda function would look nice here, but C++11 has no template lambdas. https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:184: for (const base::Callback<void()>& deferred_js_call : deferred_js_calls_) On 2016/06/07 16:43:03, xiyuan wrote: > nit: for (const auto& deferred_js_call : deferred_js_calls_) Done. https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:186: } On 2016/06/07 16:43:03, xiyuan wrote: > deferred_js_calls_.clear(); after we are done. Done. https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h (right): https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.h:167: std::vector<base::Callback<void()>> deferred_js_calls_; On 2016/06/07 16:43:03, xiyuan wrote: > nit: base::Callback<void()> -> base::Closure Done.
lgtm https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc (right): https://codereview.chromium.org/2042283002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/core_oobe_handler.cc:176: &CoreOobeHandler::ExecuteDeferredJSCall< On 2016/06/07 17:11:13, emaxx wrote: > On 2016/06/07 16:43:03, xiyuan wrote: > > Is it possible to base::Bind to CallJS directly instead of having > > ExecuteDeferredJSCall as middle man? > > I don't see an easy way, because the arguments passed to CallJSOrDefer are > const-references, i.e. pointers or references to them cannot be stored. As > base::Value is non-copyable and non-movable, then I had to use smart pointer for > storing it and passing. But CallJS cannot be called with a smart pointer, it > accepts const-references to the actual values. > > As another thought, lambda function would look nice here, but C++11 has no > template lambdas. Thanks for the explanation.
Thanks for the fast review! I added another one-line patch to add a forgotten include that is used now.
slgtm Thank you for taking care this long standing issue. :)
The CQ bit was checked by emaxx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2042283002/#ps40001 (title: "Add forgotten include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042283002/40001
Message was sent while issue was closed.
Description was changed from ========== Postpone JS calls from CoreOobeHandler until the JS side gets initialized This fixes a race between loading JavaScript code and calling methods from it. The race was caused by CoreOobeHandler class whose methods (like ReloadContent) called JavaScript methods directly, while there was a possibility that JavaScript files defining them were not loaded yet. This was observed, for example, when running tests inherited from LoginManagerTest, as the form of the following errors printed into console: * [ERROR:CONSOLE(1)] "Uncaught ReferenceError: cr is not defined", source: (1) * [ERROR:CONSOLE(1)] "Uncaught TypeError: Cannot read property 'reloadContent' of undefined", source: (1) This CL adds a new condition that doesn't allow to call JS methods until 'screenStateInitialize' message is received from JS side. Any attempts to call JS method too early are deferred and executed when the 'screenStateInitialize' is finally received. BUG=614805 TEST=manual: run login-related tests and check that no JavaScript errors appear in the console. ========== to ========== Postpone JS calls from CoreOobeHandler until the JS side gets initialized This fixes a race between loading JavaScript code and calling methods from it. The race was caused by CoreOobeHandler class whose methods (like ReloadContent) called JavaScript methods directly, while there was a possibility that JavaScript files defining them were not loaded yet. This was observed, for example, when running tests inherited from LoginManagerTest, as the form of the following errors printed into console: * [ERROR:CONSOLE(1)] "Uncaught ReferenceError: cr is not defined", source: (1) * [ERROR:CONSOLE(1)] "Uncaught TypeError: Cannot read property 'reloadContent' of undefined", source: (1) This CL adds a new condition that doesn't allow to call JS methods until 'screenStateInitialize' message is received from JS side. Any attempts to call JS method too early are deferred and executed when the 'screenStateInitialize' is finally received. BUG=614805 TEST=manual: run login-related tests and check that no JavaScript errors appear in the console. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Postpone JS calls from CoreOobeHandler until the JS side gets initialized This fixes a race between loading JavaScript code and calling methods from it. The race was caused by CoreOobeHandler class whose methods (like ReloadContent) called JavaScript methods directly, while there was a possibility that JavaScript files defining them were not loaded yet. This was observed, for example, when running tests inherited from LoginManagerTest, as the form of the following errors printed into console: * [ERROR:CONSOLE(1)] "Uncaught ReferenceError: cr is not defined", source: (1) * [ERROR:CONSOLE(1)] "Uncaught TypeError: Cannot read property 'reloadContent' of undefined", source: (1) This CL adds a new condition that doesn't allow to call JS methods until 'screenStateInitialize' message is received from JS side. Any attempts to call JS method too early are deferred and executed when the 'screenStateInitialize' is finally received. BUG=614805 TEST=manual: run login-related tests and check that no JavaScript errors appear in the console. ========== to ========== Postpone JS calls from CoreOobeHandler until the JS side gets initialized This fixes a race between loading JavaScript code and calling methods from it. The race was caused by CoreOobeHandler class whose methods (like ReloadContent) called JavaScript methods directly, while there was a possibility that JavaScript files defining them were not loaded yet. This was observed, for example, when running tests inherited from LoginManagerTest, as the form of the following errors printed into console: * [ERROR:CONSOLE(1)] "Uncaught ReferenceError: cr is not defined", source: (1) * [ERROR:CONSOLE(1)] "Uncaught TypeError: Cannot read property 'reloadContent' of undefined", source: (1) This CL adds a new condition that doesn't allow to call JS methods until 'screenStateInitialize' message is received from JS side. Any attempts to call JS method too early are deferred and executed when the 'screenStateInitialize' is finally received. BUG=614805 TEST=manual: run login-related tests and check that no JavaScript errors appear in the console. Committed: https://crrev.com/e873b56e4b1cec5bdb3ccf6e8b7ccd5e97d94c37 Cr-Commit-Position: refs/heads/master@{#398342} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e873b56e4b1cec5bdb3ccf6e8b7ccd5e97d94c37 Cr-Commit-Position: refs/heads/master@{#398342} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
