Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2494343002:
Fix for potential crash due to scheduling an empty callback. (Closed)
DescriptionFix for potential crash due to scheduling an empty callback.
Empty callback posted from:
https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_observer.cc?rcl=0&l=411
In other cases, callback is checked before use.
Review-Url: https://codereview.chromium.org/2494343002
Cr-Commit-Position: refs/heads/master@{#451977}
Committed: https://chromium.googlesource.com/chromium/src/+/6d4d9ceb2d705330ad8ac4593e28261e4254ea0a
Patch Set 1 #
Messages
Total messages: 21 (10 generated)
drbasic@yandex-team.ru changed reviewers: + cpu@chromium.org
cpu@chromium.org changed reviewers: + waffles@chromium.org
Sorry did not see this before. If only one caller can we fix the caller instead?
Is there a real crash here? The caller should only invoke this if the component updater reports that it can download the associated component (i.e. has that appid registered). The caller legitimately has no use for the callback (instead it relies on the plugin notification stack). We could pass a callback to a no-op function.
sorin@chromium.org changed reviewers: + sorin@chromium.org
lgtm Thank you and we apologize for the response time, This is good change for the time being. It is not causing a crash, mostly because the execution flow is not likely to take the offending branch. This change is sufficient since the validity of the callback is tested downstream at other call sites. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=689635 to deal with this code cleanup later on.
The CQ bit was checked by drbasic@yandex-team.ru
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 drbasic@yandex-team.ru
The CQ bit was checked by drbasic@yandex-team.ru
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by drbasic@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1487754760611780, "parent_rev":
"59826e3fda1a3ff0352043c87eb279550134d346", "commit_rev":
"6d4d9ceb2d705330ad8ac4593e28261e4254ea0a"}
Message was sent while issue was closed.
Description was changed from ========== Fix for potential crash due to scheduling an empty callback. Empty callback posted from: https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_observer.c... In other cases, callback is checked before use. ========== to ========== Fix for potential crash due to scheduling an empty callback. Empty callback posted from: https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_observer.c... In other cases, callback is checked before use. Review-Url: https://codereview.chromium.org/2494343002 Cr-Commit-Position: refs/heads/master@{#451977} Committed: https://chromium.googlesource.com/chromium/src/+/6d4d9ceb2d705330ad8ac4593e28... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/6d4d9ceb2d705330ad8ac4593e28... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
