Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
790793007:
Mojo HTML viewer crash (Closed)
|
Created:
6 years ago by dtapuska Modified:
5 years, 11 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMojo HTML viewer crash
Check that we have a valid navigator_host_ object before we call methods on it
BUG=443193
Committed: https://crrev.com/ea800be6cdc58af3b34d6b98f4a26ed35bedb671
Cr-Commit-Position: refs/heads/master@{#310090}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Change based on LazyInterfacePtr #Patch Set 3 : Add LazyInterfacePtr #Patch Set 4 : Rebase #Messages
Total messages: 20 (8 generated)
dtapuska@chromium.org changed reviewers: + davemoore@chromium.org
davemoore@chromium.org changed reviewers: + aa@chromium.org
It's not clear to me when you should be able to expect to have a navigator_host_. Deferring to Aaron. https://codereview.chromium.org/790793007/diff/1/mojo/services/html_viewer/ht... File mojo/services/html_viewer/html_document.cc (right): https://codereview.chromium.org/790793007/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_document.cc:243: if (embedder_service_provider_.get()) nit: There should be braces around this.
https://codereview.chromium.org/790793007/diff/1/mojo/services/html_viewer/ht... File mojo/services/html_viewer/html_document.cc (right): https://codereview.chromium.org/790793007/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_document.cc:243: if (embedder_service_provider_.get()) On 2014/12/19 23:56:13, DaveMoore wrote: > nit: There should be braces around this. Modify LazyInterfacePtr so that you can do navigator_host_.get(). It should first test whether service_provider_ is set, and if not, return false before doing anything else.
https://codereview.chromium.org/790793007/diff/1/mojo/services/html_viewer/ht... File mojo/services/html_viewer/html_document.cc (right): https://codereview.chromium.org/790793007/diff/1/mojo/services/html_viewer/ht... mojo/services/html_viewer/html_document.cc:243: if (embedder_service_provider_.get()) On 2014/12/20 17:46:07, Aaron Boodman wrote: > On 2014/12/19 23:56:13, DaveMoore wrote: > > nit: There should be braces around this. > > Modify LazyInterfacePtr so that you can do navigator_host_.get(). It should > first test whether service_provider_ is set, and if not, return false before > doing anything else. Done.
lgtm
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/790793007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dtapuska@chromium.org changed reviewers: + jamesr@chromium.org
jamesr@chromium.org changed reviewers: + rockot@chromium.org
The way we get new code into mojo/public/ is by doing a roll of the SDK. Pulling chunks in piecemeal is risky even if the chunk has landed in the mojo repo since the combination of pieces isn't tested upstream.
I was about to do a roll. Is this stuff already landed in mojo upstream?
On 2015/01/05 21:42:41, Ken Rockot wrote: > I was about to do a roll. Is this stuff already landed in mojo upstream? Yes the relevant parts are in mojo upstream. There is an additional file in this CL which isn't part of mojo upstream; but will rebase it once you roll it from upstream.
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/790793007/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ea800be6cdc58af3b34d6b98f4a26ed35bedb671 Cr-Commit-Position: refs/heads/master@{#310090}
Message was sent while issue was closed.
Patchset #5 (id:80001) has been deleted |
