Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1805803002:
Add checks to font proxy for diagnostics. (Closed)
|
Created:
4 years, 9 months ago by Ilya Kulshin Modified:
4 years, 9 months ago Reviewers:
ananta CC:
chromium-reviews, darin-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd checks to font proxy for diagnostics.
BUG=561873
Committed: https://crrev.com/ec3750c0ac7f203c3d78f218faf37ea1fa81ebde
Cr-Commit-Position: refs/heads/master@{#381562}
Patch Set 1 #Patch Set 2 : Relax some checks that cause unit tests to fail and are unlikely to be triggered anyway. #
Total comments: 1
Messages
Total messages: 24 (11 generated)
Description was changed from ========== Add checks to font proxy for diagnostics. BUG= ========== to ========== Add checks to font proxy for diagnostics. BUG=561873 ==========
kulshin@chromium.org changed reviewers: + jam@chromium.org
ptal. I'm adding some CHECKs to help track down bug 561873. I'm hoping that by seeing which of these get hit on canary I'll be able to diagnose the problem and revert this change. I'll also revert this change if we don't get any useful data within about a week or so.
On 2016/03/16 03:38:17, Ilya Kulshin wrote: > ptal. I'm adding some CHECKs to help track down bug 561873. I'm hoping that by > seeing which of these get hit on canary I'll be able to diagnose the problem and > revert this change. I'll also revert this change if we don't get any useful data > within about a week or so. please use specific owners, i.e. in content/browser/renderer_host (ananta for win) and one from content/child/dwrite_font_proxy/OWNERS
kulshin@chromium.org changed reviewers: - jam@chromium.org
kulshin@chromium.org changed reviewers: + ananta@chromium.org
ptal. I'm adding some CHECKs to help track down bug 561873. I'm hoping that by seeing which of these get hit on canary I'll be able to diagnose the problem and revert this change. I'll also revert this change if we don't get any useful data within about a week or so.
https://codereview.chromium.org/1805803002/diff/20001/content/browser/rendere... File content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc (right): https://codereview.chromium.org/1805803002/diff/20001/content/browser/rendere... content/browser/renderer_host/dwrite_font_proxy_message_filter_win.cc:148: CHECK(family_index >= collection_->GetFontFamilyCount()); You should use base Alias here and below to ensure that the local variables you want to monitor in the crashes are not optimized out.
Thanks for the tip. I reviewed the change, and in most cases the location of the crash would be the most significant datapoint. In cases where additional info would be needed, that is typically the HRESULT of the previous operaiton, which is checked either as part of the CHECK, or on the immediately preceeding line, so I believe it would be preserved.
The CQ bit was checked by kulshin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805803002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805803002/20001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by kulshin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805803002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805803002/20001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm
The CQ bit was checked by kulshin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805803002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805803002/20001
Message was sent while issue was closed.
Description was changed from ========== Add checks to font proxy for diagnostics. BUG=561873 ========== to ========== Add checks to font proxy for diagnostics. BUG=561873 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add checks to font proxy for diagnostics. BUG=561873 ========== to ========== Add checks to font proxy for diagnostics. BUG=561873 Committed: https://crrev.com/ec3750c0ac7f203c3d78f218faf37ea1fa81ebde Cr-Commit-Position: refs/heads/master@{#381562} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ec3750c0ac7f203c3d78f218faf37ea1fa81ebde Cr-Commit-Position: refs/heads/master@{#381562} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
