Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2276133003:
Windows: Fix an invalid handle when printing. (Closed)
DescriptionWindows: Fix an invalid handle when printing.
BUG=639505
Committed: https://crrev.com/ae78ab89c2b1d437cdbd933fac8ab15e2dbd3ebc
Cr-Commit-Position: refs/heads/master@{#414530}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add TODO #Messages
Total messages: 19 (10 generated)
The CQ bit was checked by thestig@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...
thestig@chromium.org changed reviewers: + erikchen@chromium.org, wfh@chromium.org
lgtm
https://codereview.chromium.org/2276133003/diff/1/components/printing/rendere... File components/printing/renderer/print_web_view_helper_pdf_win.cc (right): https://codereview.chromium.org/2276133003/diff/1/components/printing/rendere... components/printing/renderer/print_web_view_helper_pdf_win.cc:63: if (printed_page_params.metafile_data_handle.IsValid()) This works, but probably not the way we wanted it to. :( https://cs.chromium.org/chromium/src/base/memory/shared_memory_handle_win.cc?... SharedMemoryHandle::Close() is a const method that calls CloseHandle() on the underlying handle_. But it doesn't touch handle_. SharedMemoryHandle::IsValid just checks if handle_ is nullptr. [If we assume sane semantics for Close() and IsValid(), then the existing implementation is nonsense. That's also why the crash existed to begin with - why would it not be valid to Close a Valid handle?] I bet that all of these weird semantics stem from base::FileDescriptor. Could you add: // TODO(erikchen): Fix semantics. See https://crbug.com/640840
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yes, it's weird and my head hurts a little. https://codereview.chromium.org/2276133003/diff/1/components/printing/rendere... File components/printing/renderer/print_web_view_helper_pdf_win.cc (right): https://codereview.chromium.org/2276133003/diff/1/components/printing/rendere... components/printing/renderer/print_web_view_helper_pdf_win.cc:63: if (printed_page_params.metafile_data_handle.IsValid()) On 2016/08/25 01:20:00, erikchen wrote: > This works, but probably not the way we wanted it to. :( > > https://cs.chromium.org/chromium/src/base/memory/shared_memory_handle_win.cc?... > > SharedMemoryHandle::Close() is a const method that calls CloseHandle() on the > underlying handle_. But it doesn't touch handle_. > > SharedMemoryHandle::IsValid just checks if handle_ is nullptr. > > [If we assume sane semantics for Close() and IsValid(), then the existing > implementation is nonsense. That's also why the crash existed to begin with - > why would it not be valid to Close a Valid handle?] > > I bet that all of these weird semantics stem from base::FileDescriptor. Could > you add: > > // TODO(erikchen): Fix semantics. See https://crbug.com/640840 > Done.
The CQ bit was checked by thestig@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org Link to the patchset: https://codereview.chromium.org/2276133003/#ps20001 (title: "Add TODO")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Windows: Fix an invalid handle when printing. BUG=639505 ========== to ========== Windows: Fix an invalid handle when printing. BUG=639505 Committed: https://crrev.com/ae78ab89c2b1d437cdbd933fac8ab15e2dbd3ebc Cr-Commit-Position: refs/heads/master@{#414530} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ae78ab89c2b1d437cdbd933fac8ab15e2dbd3ebc Cr-Commit-Position: refs/heads/master@{#414530} |
