Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2431543003:
WelcomeHandler sometimes has browser_ null in ctor (Closed)
DescriptionFix WelcomeHandler crash when signin link is clicked.
BUG=656153
Committed: https://crrev.com/9ae964cc90a742835cd7c86d8c2d249dde19b4e4
Cr-Commit-Position: refs/heads/master@{#426092}
Patch Set 1 #Patch Set 2 : Fix WelcomeHandler crash when signin link is clicked. #Patch Set 3 : Fix WelcomeHandler crash when signin link is clicked. #
Total comments: 2
Patch Set 4 : Fix WelcomeHandler crash when signin link is clicked. #
Total comments: 1
Patch Set 5 : Change to GetBrowser()->ShowModalSigninWindow #Messages
Total messages: 32 (13 generated)
Description was changed from ========== WelcomHandler sometimes has browser_ null in ctor BUG=656153 ========== to ========== WelcomHandler sometimes has browser_ null in ctor BUG=656153 ==========
corona10@gmail.com changed reviewers: + tmartino@chromium.org
@tmartino PTAL
corona10@gmail.com changed reviewers: + dpapad@chromium.org
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
https://chromiumcodereview.appspot.com/2431543003/diff/40001/chrome/browser/u... File chrome/browser/ui/webui/welcome_handler.cc (right): https://chromiumcodereview.appspot.com/2431543003/diff/40001/chrome/browser/u... chrome/browser/ui/webui/welcome_handler.cc:50: // https://crbug.com/656153 +dbeam who might know more about WebUI lifetime related issues. @dbeam: Do you know if this a known issue? Does it happen on other WebUI pages?
dpapad@chromium.org changed reviewers: - dpapad@chromium.org
https://codereview.chromium.org/2431543003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_handler.cc (right): https://codereview.chromium.org/2431543003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_handler.cc:21: browser_(chrome::FindBrowserWithWebContents(web_ui->GetWebContents())), can we just make a Browser* WelcomeHandler::GetBrowser() { DCHECK(web_ui()); content::WebContents* contents = web_ui()->GetWebContents(); DCHECK(contents); Browser* browser = chrome::FindBrowserWithWebContents(contents); DCHECK(browser); return browser; } and use it everywhere instead of browser_?
On 2016/10/18 at 21:01:54, dbeam wrote: > https://codereview.chromium.org/2431543003/diff/40001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/welcome_handler.cc (right): > > https://codereview.chromium.org/2431543003/diff/40001/chrome/browser/ui/webui... > chrome/browser/ui/webui/welcome_handler.cc:21: browser_(chrome::FindBrowserWithWebContents(web_ui->GetWebContents())), > can we just make a > > Browser* WelcomeHandler::GetBrowser() { > DCHECK(web_ui()); > content::WebContents* contents = web_ui()->GetWebContents(); > DCHECK(contents); > Browser* browser = chrome::FindBrowserWithWebContents(contents); > DCHECK(browser); > return browser; > } > > and use it everywhere instead of browser_? +1 to this approach FYI this is a rather urgent fix, so if you have a chance to update your CL tonight (I'm GMT-4) we can go ahead and submit tomorrow, but otherwise I'll probably pick this bug up myself in the morning.
On 2016/10/18 21:54:26, tmartino wrote: > On 2016/10/18 at 21:01:54, dbeam wrote: > > > https://codereview.chromium.org/2431543003/diff/40001/chrome/browser/ui/webui... > > File chrome/browser/ui/webui/welcome_handler.cc (right): > > > > > https://codereview.chromium.org/2431543003/diff/40001/chrome/browser/ui/webui... > > chrome/browser/ui/webui/welcome_handler.cc:21: > browser_(chrome::FindBrowserWithWebContents(web_ui->GetWebContents())), > > can we just make a > > > > Browser* WelcomeHandler::GetBrowser() { > > DCHECK(web_ui()); > > content::WebContents* contents = web_ui()->GetWebContents(); > > DCHECK(contents); > > Browser* browser = chrome::FindBrowserWithWebContents(contents); > > DCHECK(browser); > > return browser; > > } > > > > and use it everywhere instead of browser_? > > +1 to this approach > > FYI this is a rather urgent fix, so if you have a chance to update your CL > tonight (I'm GMT-4) we can go ahead and submit tomorrow, but otherwise I'll > probably pick this bug up myself in the morning. @tmartino Thnaks I will fix it now.
On 2016/10/18 21:59:59, dhna wrote: > On 2016/10/18 21:54:26, tmartino wrote: > > On 2016/10/18 at 21:01:54, dbeam wrote: > > > > > > https://codereview.chromium.org/2431543003/diff/40001/chrome/browser/ui/webui... > > > File chrome/browser/ui/webui/welcome_handler.cc (right): > > > > > > > > > https://codereview.chromium.org/2431543003/diff/40001/chrome/browser/ui/webui... > > > chrome/browser/ui/webui/welcome_handler.cc:21: > > browser_(chrome::FindBrowserWithWebContents(web_ui->GetWebContents())), > > > can we just make a > > > > > > Browser* WelcomeHandler::GetBrowser() { > > > DCHECK(web_ui()); > > > content::WebContents* contents = web_ui()->GetWebContents(); > > > DCHECK(contents); > > > Browser* browser = chrome::FindBrowserWithWebContents(contents); > > > DCHECK(browser); > > > return browser; > > > } > > > > > > and use it everywhere instead of browser_? > > > > +1 to this approach > > > > FYI this is a rather urgent fix, so if you have a chance to update your CL > > tonight (I'm GMT-4) we can go ahead and submit tomorrow, but otherwise I'll > > probably pick this bug up myself in the morning. > > @tmartino > Thnaks I will fix it now. @tmartino I update my CL. PTAL
https://codereview.chromium.org/2431543003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/welcome_handler.cc (right): https://codereview.chromium.org/2431543003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/welcome_handler.cc:49: browser->ShowModalSigninWindow( nit: GetBrowser()->ShowModalSigninWindow(
lgtm can you update the CL description (i.e. WelcomHandler -> WelcomeHandler, "Fix crash when signin link is clicked" or something like that)
Description was changed from ========== WelcomHandler sometimes has browser_ null in ctor BUG=656153 ========== to ========== WelcomeHandler sometimes has browser_ null in ctor BUG=656153 ==========
Description was changed from ========== WelcomeHandler sometimes has browser_ null in ctor BUG=656153 ========== to ========== Fix WelcomeHandler crash when signin link is clicked. BUG=656153 ==========
On 2016/10/18 22:36:16, Dan Beam wrote: > lgtm > > can you update the CL description (i.e. WelcomHandler -> WelcomeHandler, "Fix > crash when signin link is clicked" or something like that) Thanks. I fixed it as you comment :-)
On 2016/10/18 at 22:36:16, dbeam wrote: > lgtm > > can you update the CL description (i.e. WelcomHandler -> WelcomeHandler, "Fix crash when signin link is clicked" or something like that) lgtm (pending the same fixes)
The CQ bit was checked by corona10@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/18 at 22:47:29, commit-bot wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Thanks for taking a look at this, it's a really important fix that I really haven't had the time to tackle. Please make sure you upload your patch with the fix for dbeam's nit combining lines 48-49 before you try/land this. -Tommy
The CQ bit was unchecked by corona10@gmail.com
On 2016/10/18 22:50:39, tmartino wrote: > On 2016/10/18 at 22:47:29, commit-bot wrote: > > CQ is trying da patch. Follow status at > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > Thanks for taking a look at this, it's a really important fix that I really > haven't had the time to tackle. > > Please make sure you upload your patch with the fix for dbeam's nit combining > lines 48-49 before you try/land this. > > -Tommy Okay I fix it right away.
The CQ bit was checked by corona10@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from tmartino@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2431543003/#ps80001 (title: "Change to GetBrowser()->ShowModalSigninWindow")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/18 22:52:57, dhna wrote: > On 2016/10/18 22:50:39, tmartino wrote: > > On 2016/10/18 at 22:47:29, commit-bot wrote: > > > CQ is trying da patch. Follow status at > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > Thanks for taking a look at this, it's a really important fix that I really > > haven't had the time to tackle. > > > > Please make sure you upload your patch with the fix for dbeam's nit combining > > lines 48-49 before you try/land this. > > > > -Tommy > > Okay I fix it right away. I fix for dbeam's nit combining. PTAL.
On 2016/10/18 at 22:56:53, corona10 wrote: > On 2016/10/18 22:52:57, dhna wrote: > > On 2016/10/18 22:50:39, tmartino wrote: > > > On 2016/10/18 at 22:47:29, commit-bot wrote: > > > > CQ is trying da patch. Follow status at > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > > > Thanks for taking a look at this, it's a really important fix that I really > > > haven't had the time to tackle. > > > > > > Please make sure you upload your patch with the fix for dbeam's nit combining > > > lines 48-49 before you try/land this. > > > > > > -Tommy > > > > Okay I fix it right away. > > I fix for dbeam's nit combining. > PTAL. Good stuff!
On 2016/10/18 22:58:27, tmartino wrote: > On 2016/10/18 at 22:56:53, corona10 wrote: > > On 2016/10/18 22:52:57, dhna wrote: > > > On 2016/10/18 22:50:39, tmartino wrote: > > > > On 2016/10/18 at 22:47:29, commit-bot wrote: > > > > > CQ is trying da patch. Follow status at > > > > > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > > > > > Thanks for taking a look at this, it's a really important fix that I > really > > > > haven't had the time to tackle. > > > > > > > > Please make sure you upload your patch with the fix for dbeam's nit > combining > > > > lines 48-49 before you try/land this. > > > > > > > > -Tommy > > > > > > Okay I fix it right away. > > > > I fix for dbeam's nit combining. > > PTAL. > > Good stuff! @ tmartino I was too urgent. Thank you for your careful comment :-)
Message was sent while issue was closed.
Description was changed from ========== Fix WelcomeHandler crash when signin link is clicked. BUG=656153 ========== to ========== Fix WelcomeHandler crash when signin link is clicked. BUG=656153 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix WelcomeHandler crash when signin link is clicked. BUG=656153 ========== to ========== Fix WelcomeHandler crash when signin link is clicked. BUG=656153 Committed: https://crrev.com/9ae964cc90a742835cd7c86d8c2d249dde19b4e4 Cr-Commit-Position: refs/heads/master@{#426092} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9ae964cc90a742835cd7c86d8c2d249dde19b4e4 Cr-Commit-Position: refs/heads/master@{#426092} |
