Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2848093002:
Process null capabilities as a capability fetch failure. (Closed)
DescriptionProcess null capabilities as a capability fetch failure.
We can never do anything if a request for printer capabilities returns
null. Thus, assert that it's non-null and call the failure code if capabilities is null.
This also fixes PrintPreviewDestinationSearchTest.Select which has been
failing on the Linux MSan buildbot sporadically.
BUG=708739, 716595
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2848093002
Cr-Commit-Position: refs/heads/master@{#468794}
Committed: https://chromium.googlesource.com/chromium/src/+/d61afbf71f7ccea7bf8b2e5632e3f3d052a7813f
Patch Set 1 #
Total comments: 4
Patch Set 2 : refine assertions #
Total comments: 8
Patch Set 3 : address comments #
Dependent Patchsets: Messages
Total messages: 22 (15 generated)
Description was changed from ========== Process null capabilities as a capability fetch failure. We can never do anything if a request for printer capabilities returns null. Thus, assert that it's non-null and redirect the request in native layer when there's a failure. This also fixes PrintPreviewDestinationSearchTest.Select which has been failing on the Linux MSan buildbot sporadically. BUG=708739,716595 ========== to ========== Process null capabilities as a capability fetch failure. We can never do anything if a request for printer capabilities returns null. Thus, assert that it's non-null and redirect the request in native layer when there's a failure. This also fixes PrintPreviewDestinationSearchTest.Select which has been failing on the Linux MSan buildbot sporadically. BUG=708739,716595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by skau@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...
Description was changed from ========== Process null capabilities as a capability fetch failure. We can never do anything if a request for printer capabilities returns null. Thus, assert that it's non-null and redirect the request in native layer when there's a failure. This also fixes PrintPreviewDestinationSearchTest.Select which has been failing on the Linux MSan buildbot sporadically. BUG=708739,716595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Process null capabilities as a capability fetch failure. We can never do anything if a request for printer capabilities returns null. Thus, assert that it's non-null and redirect the request in native layer when there's a failure. This also fixes PrintPreviewDestinationSearchTest.Select which has been failing on the Linux MSan buildbot sporadically. BUG=708739,716595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
skau@chromium.org changed reviewers: + dpapad@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2848093002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2848093002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/destination_store.js:1547: assert(rawCapabilities != null, assert(rawCapabilities); The comment does not add any extra information that is not already conveyed with the existence of the assert(). https://codereview.chromium.org/2848093002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/2848093002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/native_layer.js:553: if (settingsInfo.capabilities == null) { Why are we doing this in Javascript instead of C++? At [1] there is already code that checks settingsInfo and calls the error callback directly. Here we let C++ call the success callback, and redirect to the error callback ourselves. It seems unnecessary. [1] https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/print_preview/pr...
PTAL. I've incorporated the review comments. https://codereview.chromium.org/2848093002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2848093002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/data/destination_store.js:1547: assert(rawCapabilities != null, On 2017/05/01 17:39:28, dpapad wrote: > assert(rawCapabilities); > > The comment does not add any extra information that is not already conveyed with > the existence of the assert(). Done. https://codereview.chromium.org/2848093002/diff/1/chrome/browser/resources/pr... File chrome/browser/resources/print_preview/native_layer.js (right): https://codereview.chromium.org/2848093002/diff/1/chrome/browser/resources/pr... chrome/browser/resources/print_preview/native_layer.js:553: if (settingsInfo.capabilities == null) { On 2017/05/01 17:39:28, dpapad wrote: > Why are we doing this in Javascript instead of C++? At [1] there is already code > that checks settingsInfo and calls the error callback directly. Here we let C++ > call the success callback, and redirect to the error callback ourselves. It > seems unnecessary. > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/print_preview/pr... It crept up as a bug. The code you referenced only checks that settingsInfo is non-null. But in fact, we get crashes if settingsInfo.capabilities is null. However, I'll make this an assert and strengthen the check in the handler.
The CQ bit was checked by skau@chromium.org to run a CQ dry run
Description was changed from ========== Process null capabilities as a capability fetch failure. We can never do anything if a request for printer capabilities returns null. Thus, assert that it's non-null and redirect the request in native layer when there's a failure. This also fixes PrintPreviewDestinationSearchTest.Select which has been failing on the Linux MSan buildbot sporadically. BUG=708739,716595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Process null capabilities as a capability fetch failure. We can never do anything if a request for printer capabilities returns null. Thus, assert that it's non-null and call the failure code if capabilities is null. This also fixes PrintPreviewDestinationSearchTest.Select which has been failing on the Linux MSan buildbot sporadically. BUG=708739,716595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2848093002/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2848093002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1563: rawCapabilities); If we already assert in native_layer.js before triggering the CAPABILITIES_SET event, this assertion here is redundant. Can we stick with an assertion either here or in native_layer.js but not in both? https://codereview.chromium.org/2848093002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2848093002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1281: // Validate that |settings_info| is valid for the javascript. Nit // Check that |settings_info| is valid. https://codereview.chromium.org/2848093002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview_destination_search_test.js (right): https://codereview.chromium.org/2848093002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview_destination_search_test.js:218: test('ReceiveFailedSetup', function() { if (cr.isChromeOS) { test('ReceiveFailedSetup', function() { ... }); } https://codereview.chromium.org/2848093002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview_destination_search_test.js:220: // The 'ResolutionFails' test covers this case for non-CrOS. ResolutionFails test also returns early for non-CrOS. Is this comment accurate?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Comments addressed. PTAL. https://codereview.chromium.org/2848093002/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2848093002/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1563: rawCapabilities); On 2017/05/02 00:07:41, dpapad wrote: > If we already assert in native_layer.js before triggering the CAPABILITIES_SET > event, this assertion here is redundant. Can we stick with an assertion either > here or in native_layer.js but not in both? Done. https://codereview.chromium.org/2848093002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2848093002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1281: // Validate that |settings_info| is valid for the javascript. On 2017/05/02 00:07:41, dpapad wrote: > Nit > > // Check that |settings_info| is valid. Done. https://codereview.chromium.org/2848093002/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview_destination_search_test.js (right): https://codereview.chromium.org/2848093002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview_destination_search_test.js:218: test('ReceiveFailedSetup', function() { On 2017/05/02 00:07:41, dpapad wrote: > if (cr.isChromeOS) { > test('ReceiveFailedSetup', function() { > ... > }); > } Done. https://codereview.chromium.org/2848093002/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/print_preview_destination_search_test.js:220: // The 'ResolutionFails' test covers this case for non-CrOS. On 2017/05/02 00:07:41, dpapad wrote: > ResolutionFails test also returns early for non-CrOS. Is this comment accurate? The comment is accurate. The other test is disabled due to a bug because it logs an error and causes the test to fail.
LGTM
The CQ bit was checked by skau@chromium.org
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": 40001, "attempt_start_ts": 1493757321830360,
"parent_rev": "26f0a139eaac0d4554cd83c9b736724d2c6553be", "commit_rev":
"d61afbf71f7ccea7bf8b2e5632e3f3d052a7813f"}
Message was sent while issue was closed.
Description was changed from ========== Process null capabilities as a capability fetch failure. We can never do anything if a request for printer capabilities returns null. Thus, assert that it's non-null and call the failure code if capabilities is null. This also fixes PrintPreviewDestinationSearchTest.Select which has been failing on the Linux MSan buildbot sporadically. BUG=708739,716595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Process null capabilities as a capability fetch failure. We can never do anything if a request for printer capabilities returns null. Thus, assert that it's non-null and call the failure code if capabilities is null. This also fixes PrintPreviewDestinationSearchTest.Select which has been failing on the Linux MSan buildbot sporadically. BUG=708739,716595 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2848093002 Cr-Commit-Position: refs/heads/master@{#468794} Committed: https://chromium.googlesource.com/chromium/src/+/d61afbf71f7ccea7bf8b2e5632e3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d61afbf71f7ccea7bf8b2e5632e3... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
