Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2361513003:
MD History: Update sign in state in data source (Closed)
|
Created:
4 years, 3 months ago by lshang Modified:
4 years, 2 months ago CC:
chromium-reviews, asanka, Patrick Dubroy, michaelpg+watch-md-ui_chromium.org, dbeam+watch-history_chromium.org, pam+watch_chromium.org, arv+watch_chromium.org, dbeam+watch-downloads_chromium.org, chrome-apps-syd-reviews_chromium.org, tsergeant Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD History: Update sign in state in data source
Currently we get user's sign in state from LoadTimeData() on page load, but
WebUI data source is re-used when refreshing the page, which will cause bugs
if user changes sign in state(e.g. via sign in promo) and reload the page
because LoadTimeData() will return out-dated sign in state.
We made a quick fix before by chrome.sending 'otherDevicesInitialized' after
synced device manager is attached to the document, so that signInStateChanged()
gets called to update element's current state. But this would cause a *fake*
sign-in action(when user signs in via promo and reload, signInState will first
be set false from LoadTimeData and then changes to be true from
otherDevicesInitialized) and 'Loading' message will keeps showing if no other
actions trigger updateSyncedDevices() to stop the fetching state.
So in this CL, we update the sign in state in WebUI data source, when profile
info changed in HistoryLoginHandler. This will guarantee that LoadTimeData()
returns updated state every time page gets loaded.
BUG=639811
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/93a39a3cfff736b8e10f65e9d7bf2612186a26c8
Cr-Commit-Position: refs/heads/master@{#422038}
Patch Set 1 #Patch Set 2 : remove downloads #
Total comments: 8
Patch Set 3 : rewrite #Patch Set 4 : remove extra line #Patch Set 5 : try another approach #
Total comments: 9
Patch Set 6 : use callback #Patch Set 7 : debug crash #Patch Set 8 : fix promo crash #
Total comments: 8
Patch Set 9 : add get method #
Total comments: 12
Patch Set 10 : just create #Patch Set 11 : remove unused forward declare #
Total comments: 4
Patch Set 12 : address #
Total comments: 2
Patch Set 13 : move #Messages
Total messages: 41 (18 generated)
Description was changed from ========== MD History: Update sign in state in data source Merge branch 'master' into MDH_loading_after_hide_links add more logs Merge branch 'master' into MDH_loading_after_hide_links add some debug info BUG= ========== to ========== MD History: Update sign in state in data source Merge branch 'master' into MDH_loading_after_hide_links add more logs Merge branch 'master' into MDH_loading_after_hide_links add some debug info BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD History: Update sign in state in data source Merge branch 'master' into MDH_loading_after_hide_links add more logs Merge branch 'master' into MDH_loading_after_hide_links add some debug info BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Update sign in state in data source Currently we get user's sign in state from LoadTimeData() on page load, but WebUI data source is re-used when refreshing the page, which will cause bugs if user changes sign in state(e.g. via sign in promo) and reload the page because LoadTimeData() will return out-dated sign in state. We made a quick fix before by chrome.sending 'otherDevicesInitialized' after synced device manager is attached to the document, so that signInStateChanged() gets called to update element's current state. But this would cause a *fake* sign-in action(when user signs in via promo and reload, signInState will first be set false from LoadTimeData and then changes to be true from otherDevicesInitialized) and 'Loading' message will keeps showing because no other actions trigger updateSyncedDevices() to stop the fetching state. So in this CL, we update the sign in state in WebUI data source, when profile info changed in HistoryLoginHandler. This will guarantee that LoadTimeData() returns updated state every time page gets loaded. BUG=639811 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD History: Update sign in state in data source Currently we get user's sign in state from LoadTimeData() on page load, but WebUI data source is re-used when refreshing the page, which will cause bugs if user changes sign in state(e.g. via sign in promo) and reload the page because LoadTimeData() will return out-dated sign in state. We made a quick fix before by chrome.sending 'otherDevicesInitialized' after synced device manager is attached to the document, so that signInStateChanged() gets called to update element's current state. But this would cause a *fake* sign-in action(when user signs in via promo and reload, signInState will first be set false from LoadTimeData and then changes to be true from otherDevicesInitialized) and 'Loading' message will keeps showing because no other actions trigger updateSyncedDevices() to stop the fetching state. So in this CL, we update the sign in state in WebUI data source, when profile info changed in HistoryLoginHandler. This will guarantee that LoadTimeData() returns updated state every time page gets loaded. BUG=639811 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Update sign in state in data source Currently we get user's sign in state from LoadTimeData() on page load, but WebUI data source is re-used when refreshing the page, which will cause bugs if user changes sign in state(e.g. via sign in promo) and reload the page because LoadTimeData() will return out-dated sign in state. We made a quick fix before by chrome.sending 'otherDevicesInitialized' after synced device manager is attached to the document, so that signInStateChanged() gets called to update element's current state. But this would cause a *fake* sign-in action(when user signs in via promo and reload, signInState will first be set false from LoadTimeData and then changes to be true from otherDevicesInitialized) and 'Loading' message will keeps showing because no other actions trigger updateSyncedDevices() to stop the fetching state. So in this CL, we update the sign in state in WebUI data source, when profile info changed in HistoryLoginHandler. This will guarantee that LoadTimeData() returns updated state every time page gets loaded. BUG=639811 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD History: Update sign in state in data source Currently we get user's sign in state from LoadTimeData() on page load, but WebUI data source is re-used when refreshing the page, which will cause bugs if user changes sign in state(e.g. via sign in promo) and reload the page because LoadTimeData() will return out-dated sign in state. We made a quick fix before by chrome.sending 'otherDevicesInitialized' after synced device manager is attached to the document, so that signInStateChanged() gets called to update element's current state. But this would cause a *fake* sign-in action(when user signs in via promo and reload, signInState will first be set false from LoadTimeData and then changes to be true from otherDevicesInitialized) and 'Loading' message will keeps showing because no other actions trigger updateSyncedDevices() to stop the fetching state. So in this CL, we update the sign in state in WebUI data source, when profile info changed in HistoryLoginHandler. This will guarantee that LoadTimeData() returns updated state every time page gets loaded. BUG=639811 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Update sign in state in data source Currently we get user's sign in state from LoadTimeData() on page load, but WebUI data source is re-used when refreshing the page, which will cause bugs if user changes sign in state(e.g. via sign in promo) and reload the page because LoadTimeData() will return out-dated sign in state. We made a quick fix before by chrome.sending 'otherDevicesInitialized' after synced device manager is attached to the document, so that signInStateChanged() gets called to update element's current state. But this would cause a *fake* sign-in action(when user signs in via promo and reload, signInState will first be set false from LoadTimeData and then changes to be true from otherDevicesInitialized) and 'Loading' message will keeps showing if no other actions trigger updateSyncedDevices() to stop the fetching state. So in this CL, we update the sign in state in WebUI data source, when profile info changed in HistoryLoginHandler. This will guarantee that LoadTimeData() returns updated state every time page gets loaded. BUG=639811 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
lshang@chromium.org changed reviewers: + calamity@chromium.org
PTAL thanks!
https://codereview.chromium.org/2361513003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_login_handler.cc (right): https://codereview.chromium.org/2361513003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_login_handler.cc:54: web_ui()->CallJavascriptFunctionUnsafe("updateSignInState", Can we just update what we need to with this updateSignInState call? I think reloading the whole WebUIDataSource here is really weird.
https://codereview.chromium.org/2361513003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_login_handler.cc (right): https://codereview.chromium.org/2361513003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_login_handler.cc:14: #include "chrome/browser/ui/webui/md_history_ui.cc" whoa whoa whoa, don't include a .cc https://codereview.chromium.org/2361513003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_login_handler.cc:50: content::WebUIDataSource* data_source = CreateMdHistoryUIHTMLSource(profile); CreateMdHistoryUIHTMLSource is private to the .cc https://codereview.chromium.org/2361513003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_login_handler.cc:54: web_ui()->CallJavascriptFunctionUnsafe("updateSignInState", On 2016/09/22 08:16:28, calamity wrote: > Can we just update what we need to with this updateSignInState call? I think > reloading the whole WebUIDataSource here is really weird. yes, it's safe (AFAIK) to just call data_source_->AddBoolean("isUserSignedIn", signed_in); that's what I'm doing in MdHistoryUI when a promo is viewed
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2361513003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_login_handler.cc (right): https://codereview.chromium.org/2361513003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_login_handler.cc:14: #include "chrome/browser/ui/webui/md_history_ui.cc" On 2016/09/22 18:18:52, Dan Beam wrote: > whoa whoa whoa, don't include a .cc i accidentally did this the other day, btw, and it wont link on many builders (duplicate symbols)
Hi Chris & Dan, I've rewritten this fix. Now the approach is:
updateSignInState(JS) gets called when profile info changed -->
chrome.send('signInStateUpdated')(JS) -->
MDHistoryUI handles the message and updates state in data source(C++) -->
next time LoadTimeData(JS) will return up-to-date sign in state.
PTALA thanks! (●'◡'●)
https://codereview.chromium.org/2361513003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/history_login_handler.cc (right):
https://codereview.chromium.org/2361513003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/history_login_handler.cc:14: #include
"chrome/browser/ui/webui/md_history_ui.cc"
On 2016/09/22 18:18:52, Dan Beam wrote:
> whoa whoa whoa, don't include a .cc
Definitely a typo! (o_O)!
https://codereview.chromium.org/2361513003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/history_login_handler.cc:50: content::WebUIDataSource*
data_source = CreateMdHistoryUIHTMLSource(profile);
On 2016/09/22 18:18:53, Dan Beam wrote:
> CreateMdHistoryUIHTMLSource is private to the .cc
Done. Removed changes in this file.
https://codereview.chromium.org/2361513003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/history_login_handler.cc:54:
web_ui()->CallJavascriptFunctionUnsafe("updateSignInState",
On 2016/09/22 18:18:52, Dan Beam wrote:
> On 2016/09/22 08:16:28, calamity wrote:
> > Can we just update what we need to with this updateSignInState call? I think
> > reloading the whole WebUIDataSource here is really weird.
>
> yes, it's safe (AFAIK) to just call
>
> data_source_->AddBoolean("isUserSignedIn", signed_in);
>
> that's what I'm doing in MdHistoryUI when a promo is viewed
Yeah we should register a new message callback to handle this in MDHistoryUI~
I think this flow is kind of in reverse. the C++ knows the signed in state more canonically. make it observe signed in state and update it's own data source to be correct if the page is reload. this leaves notifying the JS when the C++ changes. to do this in modern webuis we use addWebUIListener. https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people... https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/profile... so it'd be something like: JS behaviors: [WebUIListener], attached: function() { this.addWebUIListener('signed-in-changed', this.signedInChanged_.bind(this)); }, signedInChanged_: function(signedIn) { this.signedIn_ = signedIn; }, C++ MyHandlerOrWebUI() { scoped_thinger_observer_.Add(this) } OnSignedInStateChanged(book signed_in) { data_source_->AddBoolean("isSignedIn", signed_in); CallJavascriptFunction("cr.webUIListenerCallback", "name-of-webui-listener", base::FundamentalValue(signed_in)); }
On 2016/09/23 06:12:16, Dan Beam wrote: > I think this flow is kind of in reverse. > > the C++ knows the signed in state more canonically. make it observe signed in > state and update it's own data source to be correct if the page is reload. > > this leaves notifying the JS when the C++ changes. > > to do this in modern webuis we use addWebUIListener. > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people... > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/profile... > > so it'd be something like: > > JS > > behaviors: [WebUIListener], > > attached: function() { > this.addWebUIListener('signed-in-changed', > this.signedInChanged_.bind(this)); > }, > > signedInChanged_: function(signedIn) { > this.signedIn_ = signedIn; > }, > > > C++ > > MyHandlerOrWebUI() { > scoped_thinger_observer_.Add(this) > } > > OnSignedInStateChanged(book signed_in) { > data_source_->AddBoolean("isSignedIn", signed_in); > CallJavascriptFunction("cr.webUIListenerCallback", > "name-of-webui-listener", > base::FundamentalValue(signed_in)); > } Hey Dan, please take a look at the newest patch. I changed it a bit according to your suggestion, but there are couple of things I'm not sure of: 1. The newest patch will cause crashes when sign in state changes. I tried to AllowJavacript() before CallJavascriptFunction(), but it still DCHECK failed in https://cs.chromium.org/chromium/src/content/browser/webui/web_ui_impl.cc?l=172 trying to get target frame. 2. If we change to use WebUIListener in HistoryLoginHandler, then we probably also need to change old history page's JS part, and I'm not sure if it can applied to non-Polymer thing? 3. The current HistoryLoginHandler seems doing the same thing, which is: call into JS functions when profile info changed, as in https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/history_login_ha.... So how about we just pass in the data_source and update the boolean?
https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_login_handler.cc (right): https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_login_handler.cc:48: AllowJavascript(); you only want to call AllowJavascript() when the page tells the handler it's ready. how is that guaranteed here? I think the profile info could change before the page is ready or while it's reloading (which means it might be ignored or be run on the wrong URL [i.e. any domain] or crash chrome)
https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/synced_device_manager.js (left): https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:71: chrome.send('otherDevicesInitialized'); You can keep this in and then AllowJavascript() in the handler. You'll need to check IsJavascriptAllowed() before calling as well. https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_login_handler.cc (right): https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_login_handler.cc:48: AllowJavascript(); On 2016/09/27 23:16:23, Dan Beam wrote: > you only want to call AllowJavascript() when the page tells the handler it's > ready. how is that guaranteed here? I think the profile info could change > before the page is ready or while it's reloading (which means it might be > ignored or be run on the wrong URL [i.e. any domain] or crash chrome) Hmm, the other handlers in history don't have initialization callbacks either. In fact, that's probably why we get those random 'setForeignSessions is undefined' errors on load.
https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/app.js (right): https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/app.js:113: this.updateSignInState.bind(this)); this part is perfect :) https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_login_handler.cc (right): https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_login_handler.cc:20: HistoryLoginHandler::HistoryLoginHandler(content::WebUIDataSource* data_source) could we just pass a base::Callback instead of a WebUIDataSource*? i.e. #include "base/callback_forward.h" class HistoryLoginHandler { private: base::Callback<void(bool)> signin_callback_; }; #include "base/callback.h" HistoryLoginHandler::HistoryLoginHandler( const base::Callback<void(bool)>& signin_callback) : signin_callback_(signin_callback) {} void HistoryLoginHandler::ProfileInfoChanged() { bool signed_in = !profile_info_watcher_->GetAuthenticatedUsername().empty(); if (!signin_callback_.is_null()) signin_callback_.Run(signed_in); } and each HistoryUI creates a handler like this: HistoryUI::HistoryUI(content::WebUI* web_ui) { web_ui->AddMessageHandler(new HistoryLoginHandler( base::Bind(&HistoryUI::SignInChanged, base::Unretained(this))); // or use a WeakPtr if you're not sure about lifetime issues. } void HistoryUI::SignInChanged(bool signed_in) { data_source_->AddBoolean(kUserSignedInKey, signed_in); } https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_login_handler.cc:47: data_source_->AddBoolean("isUserSignedIn", signed_in); this also duplicates the name of this key (not so great) and provides access to the data source (preferable to not do if possible)
On 2016/09/28 02:54:23, calamity wrote: > https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/resource... > File chrome/browser/resources/md_history/synced_device_manager.js (left): > > https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/resource... > chrome/browser/resources/md_history/synced_device_manager.js:71: > chrome.send('otherDevicesInitialized'); > You can keep this in and then AllowJavascript() in the handler. You'll need to > check IsJavascriptAllowed() before calling as well. > > https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/history_login_handler.cc (right): > > https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/ui/webui... > chrome/browser/ui/webui/history_login_handler.cc:48: AllowJavascript(); > On 2016/09/27 23:16:23, Dan Beam wrote: > > you only want to call AllowJavascript() when the page tells the handler it's > > ready. how is that guaranteed here? I think the profile info could change > > before the page is ready or while it's reloading (which means it might be > > ignored or be run on the wrong URL [i.e. any domain] or crash chrome) > > Hmm, the other handlers in history don't have initialization callbacks either. > In fact, that's probably why we get those random 'setForeignSessions is > undefined' errors on load. yes, and you'd get browser crashes if one of these messages was attempted to be sent while reloading or navigating (potentially), if jochen@ hadn't disabled them
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Dan, please take a look at the newest patch and the comments I added on some lines. I debugged a bit to fix crashes of the previous solution, and got some new findings of WebUIDataSource. Let me know if you're OK with the new approach :-) https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/resource... File chrome/browser/resources/md_history/synced_device_manager.js (left): https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/resource... chrome/browser/resources/md_history/synced_device_manager.js:71: chrome.send('otherDevicesInitialized'); On 2016/09/28 02:54:23, calamity wrote: > You can keep this in and then AllowJavascript() in the handler. You'll need to > check IsJavascriptAllowed() before calling as well. Done. Good idea https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/history_login_handler.cc (right): https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_login_handler.cc:20: HistoryLoginHandler::HistoryLoginHandler(content::WebUIDataSource* data_source) On 2016/09/28 03:56:49, Dan Beam wrote: > could we just pass a base::Callback instead of a WebUIDataSource*? > > i.e. > > #include "base/callback_forward.h" > > class HistoryLoginHandler { > private: > base::Callback<void(bool)> signin_callback_; > }; > > #include "base/callback.h" > > HistoryLoginHandler::HistoryLoginHandler( > const base::Callback<void(bool)>& signin_callback) > : signin_callback_(signin_callback) {} > > void HistoryLoginHandler::ProfileInfoChanged() { > bool signed_in = !profile_info_watcher_->GetAuthenticatedUsername().empty(); > if (!signin_callback_.is_null()) > signin_callback_.Run(signed_in); > } > > and each HistoryUI creates a handler like this: > > HistoryUI::HistoryUI(content::WebUI* web_ui) { > web_ui->AddMessageHandler(new HistoryLoginHandler( > base::Bind(&HistoryUI::SignInChanged, base::Unretained(this))); > // or use a WeakPtr if you're not sure about lifetime issues. > } > > void HistoryUI::SignInChanged(bool signed_in) { > data_source_->AddBoolean(kUserSignedInKey, signed_in); > } Done. https://codereview.chromium.org/2361513003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/history_login_handler.cc:47: data_source_->AddBoolean("isUserSignedIn", signed_in); On 2016/09/28 03:56:49, Dan Beam wrote: > this also duplicates the name of this key (not so great) and provides access to > the data source (preferable to not do if possible) NaN after we re-create the data source here and this name is not needed here. https://codereview.chromium.org/2361513003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:276: content::WebUIDataSource::Add(profile, data_source_); I also modified here due to the same reason as below. Crash is seen when: 1. open one tab(not showing the promo) 2. open another tab, narrow the screent to make the promo shown 3. go back to the first tab **without** reloading, make the promo shown as well They are all because WebUIDataSource gets destroyed and replaced between multiple tabs. https://codereview.chromium.org/2361513003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:284: content::WebUIDataSource::Add(profile, data_source_); I found that purely AddBoolean() here will cause browser crashes when: 1. Open a tab, go to chrome://history 2. Open another tab, sign in/out I debugged it a bit. When opening a new tab, the browser will create a new WebUIDataSource, and this data source will replace first tab's data source, so the first one is destroyed. But when this callback method get runned, it still try to call the already-been-replaced data source and segmentation fault occurs. My solution here is re-create the data source of the current tab, so we don't need to do AddBoolean() here again in this way.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2361513003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:227: &MdHistoryUI::SignInChanged, weak_ptr_factory_.GetWeakPtr()))); you can use weakptr, but now that we're not dependent on the datasource's lifecycle i'm fairly sure base::Unretained() will work fine as each handler is owned by a webui (i.e. this class owns the HistoryLoginHandler) https://codereview.chromium.org/2361513003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:230: data_source_ = CreateMdHistoryUIHTMLSource(profile, use_test_title_); don't keep this pointer around, as it's potentially garbage if it's been overwritten (which can lead to crashes) https://codereview.chromium.org/2361513003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:284: content::WebUIDataSource::Add(profile, data_source_); On 2016/09/29 08:06:48, lshang wrote: > I found that purely AddBoolean() here will cause browser crashes when: > > 1. Open a tab, go to chrome://history > 2. Open another tab, sign in/out > > I debugged it a bit. When opening a new tab, the browser will create a new > WebUIDataSource, and this data source will replace first tab's data source, so > the first one is destroyed. But when this callback method get runned, it still > try to call the already-been-replaced data source and segmentation fault occurs. > > My solution here is re-create the data source of the current tab, so we don't > need to do AddBoolean() here again in this way. wow, that's crazy sauce. we should just make handlers not nuke eachothers sources or add a Get() method to get the current data source
https://codereview.chromium.org/2361513003/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:227: &MdHistoryUI::SignInChanged, weak_ptr_factory_.GetWeakPtr()))); On 2016/09/29 18:13:33, Dan Beam wrote: > you can use weakptr, but now that we're not dependent on the datasource's > lifecycle i'm fairly sure base::Unretained() will work fine as each handler is > owned by a webui (i.e. this class owns the HistoryLoginHandler) Done. https://codereview.chromium.org/2361513003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:230: data_source_ = CreateMdHistoryUIHTMLSource(profile, use_test_title_); On 2016/09/29 18:13:33, Dan Beam wrote: > don't keep this pointer around, as it's potentially garbage if it's been > overwritten (which can lead to crashes) Done. Added a Get() method to create the data source so that the data source pointer it returns are always valid. https://codereview.chromium.org/2361513003/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:284: content::WebUIDataSource::Add(profile, data_source_); On 2016/09/29 18:13:33, Dan Beam wrote: > On 2016/09/29 08:06:48, lshang wrote: > > I found that purely AddBoolean() here will cause browser crashes when: > > > > 1. Open a tab, go to chrome://history > > 2. Open another tab, sign in/out > > > > I debugged it a bit. When opening a new tab, the browser will create a new > > WebUIDataSource, and this data source will replace first tab's data source, so > > the first one is destroyed. But when this callback method get runned, it still > > try to call the already-been-replaced data source and segmentation fault > occurs. > > > > My solution here is re-create the data source of the current tab, so we don't > > need to do AddBoolean() here again in this way. > > wow, that's crazy sauce. we should just make handlers not nuke eachothers > sources or add a Get() method to get the current data source Done.
https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/history_login_handler.cc (right): https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_login_handler.cc:9: #include "base/callback_forward.h" base/callback.h https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/history_login_handler.h (right): https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_login_handler.h:9: base/callback_forward.h https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_ui.cc:63: content::WebUIDataSource* CreateHistoryUIHTMLSource(Profile* profile) { is there a reason why this is an anonymous method rather than an instance method? if this was an instance method, it'd have access to web_ui() and hence able to get the profile https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_ui.cc:219: return data_source; so, groby@ is going to make this API take a std::unique_ptr to make it clear that this method takes ownership of this memory and that we shouldn't keep raw pointers around if they're gonna be deleted from under us. so don't return the pointer from this method. also, I don't see where the return value is used in this method anyways... https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:279: CreateAndGetDataSource()->AddBoolean(kShowMenuPromoKey, false); I think you can just name this CreateDataSource() as setting this pref changes the value when it's created (i.e. CreateMdHistoryUIHTMLSource() checks the pref that's being changed ^ here)
https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/history_login_handler.cc (right): https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_login_handler.cc:9: #include "base/callback_forward.h" On 2016/09/30 00:47:43, Dan Beam wrote: > base/callback.h Done. https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/history_login_handler.h (right): https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_login_handler.h:9: On 2016/09/30 00:47:43, Dan Beam wrote: > base/callback_forward.h Done. https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_ui.cc:63: content::WebUIDataSource* CreateHistoryUIHTMLSource(Profile* profile) { On 2016/09/30 00:47:43, Dan Beam wrote: > is there a reason why this is an anonymous method rather than an instance > method? if this was an instance method, it'd have access to web_ui() and hence > able to get the profile Probably in order to enclose this function inside this file? There is also a saying that anonymous namespace can improve performance since symbols within the namespace do not need any external linkage and the compiler is freer to perform aggressive optimization of the code within the namespace. ExtensionsUI, MdDownloadsUI use it as anonymous as well. I'm not sure which design is better, but I'm happy to change it if you feel strongly about it:-) https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_ui.cc:219: return data_source; On 2016/09/30 00:47:43, Dan Beam wrote: > so, groby@ is going to make this API take a std::unique_ptr to make it clear > that this method takes ownership of this memory and that we shouldn't keep raw > pointers around if they're gonna be deleted from under us. so don't return the > pointer from this method. also, I don't see where the return value is used in > this method anyways... Changed to just create it. groby@'s change would be great to avoid crashes, should I wait for her CL to land first? https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:279: CreateAndGetDataSource()->AddBoolean(kShowMenuPromoKey, false); On 2016/09/30 00:47:43, Dan Beam wrote: > I think you can just name this CreateDataSource() as setting this pref changes > the value when it's created (i.e. CreateMdHistoryUIHTMLSource() checks the pref > that's being changed ^ here) Yeah you're right. Done!
lgtm https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_ui.cc:219: return data_source; On 2016/09/30 01:31:08, lshang wrote: > On 2016/09/30 00:47:43, Dan Beam wrote: > > so, groby@ is going to make this API take a std::unique_ptr to make it clear > > that this method takes ownership of this memory and that we shouldn't keep raw > > pointers around if they're gonna be deleted from under us. so don't return > the > > pointer from this method. also, I don't see where the return value is used in > > this method anyways... > > Changed to just create it. groby@'s change would be great to avoid crashes, > should I wait for her CL to land first? let's just re-create every time with a TODO for now https://codereview.chromium.org/2361513003/diff/190001/chrome/browser/ui/webu... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/190001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_ui.cc:187: base::Bind(&HistoryUI::SignInChanged, base::Unretained(this)))); why not just bind to CreateDataSource directly? https://codereview.chromium.org/2361513003/diff/190001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/190001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:225: base::Bind(&MdHistoryUI::SignInChanged, base::Unretained(this)))); same
https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_ui.cc:219: return data_source; On 2016/09/30 02:55:40, Dan Beam wrote: > On 2016/09/30 01:31:08, lshang wrote: > > On 2016/09/30 00:47:43, Dan Beam wrote: > > > so, groby@ is going to make this API take a std::unique_ptr to make it clear > > > that this method takes ownership of this memory and that we shouldn't keep > raw > > > pointers around if they're gonna be deleted from under us. so don't return > > the > > > pointer from this method. also, I don't see where the return value is used > in > > > this method anyways... > > > > Changed to just create it. groby@'s change would be great to avoid crashes, > > should I wait for her CL to land first? > > let's just re-create every time with a TODO for now Done. https://codereview.chromium.org/2361513003/diff/190001/chrome/browser/ui/webu... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/190001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_ui.cc:187: base::Bind(&HistoryUI::SignInChanged, base::Unretained(this)))); On 2016/09/30 02:55:40, Dan Beam wrote: > why not just bind to CreateDataSource directly? Done. https://codereview.chromium.org/2361513003/diff/190001/chrome/browser/ui/webu... File chrome/browser/ui/webui/md_history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/190001/chrome/browser/ui/webu... chrome/browser/ui/webui/md_history_ui.cc:225: base::Bind(&MdHistoryUI::SignInChanged, base::Unretained(this)))); On 2016/09/30 02:55:40, Dan Beam wrote: > same Done.
lgtm https://codereview.chromium.org/2361513003/diff/210001/chrome/browser/ui/webu... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/210001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_ui.cc:180: CreateDataSource(); May as well move this back to where it was. I'm vaguely worried the block of code below will affect the creation so let's just leave it where it was.
https://codereview.chromium.org/2361513003/diff/210001/chrome/browser/ui/webu... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/2361513003/diff/210001/chrome/browser/ui/webu... chrome/browser/ui/webui/history_ui.cc:180: CreateDataSource(); On 2016/09/30 04:09:49, calamity wrote: > May as well move this back to where it was. I'm vaguely worried the block of > code below will affect the creation so let's just leave it where it was. Yeah it makes sense. Done.
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, calamity@chromium.org Link to the patchset: https://codereview.chromium.org/2361513003/#ps230001 (title: "move")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== MD History: Update sign in state in data source Currently we get user's sign in state from LoadTimeData() on page load, but WebUI data source is re-used when refreshing the page, which will cause bugs if user changes sign in state(e.g. via sign in promo) and reload the page because LoadTimeData() will return out-dated sign in state. We made a quick fix before by chrome.sending 'otherDevicesInitialized' after synced device manager is attached to the document, so that signInStateChanged() gets called to update element's current state. But this would cause a *fake* sign-in action(when user signs in via promo and reload, signInState will first be set false from LoadTimeData and then changes to be true from otherDevicesInitialized) and 'Loading' message will keeps showing if no other actions trigger updateSyncedDevices() to stop the fetching state. So in this CL, we update the sign in state in WebUI data source, when profile info changed in HistoryLoginHandler. This will guarantee that LoadTimeData() returns updated state every time page gets loaded. BUG=639811 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Update sign in state in data source Currently we get user's sign in state from LoadTimeData() on page load, but WebUI data source is re-used when refreshing the page, which will cause bugs if user changes sign in state(e.g. via sign in promo) and reload the page because LoadTimeData() will return out-dated sign in state. We made a quick fix before by chrome.sending 'otherDevicesInitialized' after synced device manager is attached to the document, so that signInStateChanged() gets called to update element's current state. But this would cause a *fake* sign-in action(when user signs in via promo and reload, signInState will first be set false from LoadTimeData and then changes to be true from otherDevicesInitialized) and 'Loading' message will keeps showing if no other actions trigger updateSyncedDevices() to stop the fetching state. So in this CL, we update the sign in state in WebUI data source, when profile info changed in HistoryLoginHandler. This will guarantee that LoadTimeData() returns updated state every time page gets loaded. BUG=639811 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #13 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== MD History: Update sign in state in data source Currently we get user's sign in state from LoadTimeData() on page load, but WebUI data source is re-used when refreshing the page, which will cause bugs if user changes sign in state(e.g. via sign in promo) and reload the page because LoadTimeData() will return out-dated sign in state. We made a quick fix before by chrome.sending 'otherDevicesInitialized' after synced device manager is attached to the document, so that signInStateChanged() gets called to update element's current state. But this would cause a *fake* sign-in action(when user signs in via promo and reload, signInState will first be set false from LoadTimeData and then changes to be true from otherDevicesInitialized) and 'Loading' message will keeps showing if no other actions trigger updateSyncedDevices() to stop the fetching state. So in this CL, we update the sign in state in WebUI data source, when profile info changed in HistoryLoginHandler. This will guarantee that LoadTimeData() returns updated state every time page gets loaded. BUG=639811 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD History: Update sign in state in data source Currently we get user's sign in state from LoadTimeData() on page load, but WebUI data source is re-used when refreshing the page, which will cause bugs if user changes sign in state(e.g. via sign in promo) and reload the page because LoadTimeData() will return out-dated sign in state. We made a quick fix before by chrome.sending 'otherDevicesInitialized' after synced device manager is attached to the document, so that signInStateChanged() gets called to update element's current state. But this would cause a *fake* sign-in action(when user signs in via promo and reload, signInState will first be set false from LoadTimeData and then changes to be true from otherDevicesInitialized) and 'Loading' message will keeps showing if no other actions trigger updateSyncedDevices() to stop the fetching state. So in this CL, we update the sign in state in WebUI data source, when profile info changed in HistoryLoginHandler. This will guarantee that LoadTimeData() returns updated state every time page gets loaded. BUG=639811 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/93a39a3cfff736b8e10f65e9d7bf2612186a26c8 Cr-Commit-Position: refs/heads/master@{#422038} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/93a39a3cfff736b8e10f65e9d7bf2612186a26c8 Cr-Commit-Position: refs/heads/master@{#422038} |
