Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779433002:
[IndexedDB] Abort transaction on unknown blob (Closed)
Description[IndexedDB] Abort transaction on unknown blobs instead of crashing
There is a current refcounting known issue where a blob can be sent to
another process but while that IPC is scheduled the source process
dies. The blob will be non-existent. We want to gracefully handle this
error in IndexedDB instead of crashing.
(also fixes missing include in the connection class)
BUG=677780
Review-Url: https://codereview.chromium.org/2779433002
Cr-Commit-Position: refs/heads/master@{#460491}
Committed: https://chromium.googlesource.com/chromium/src/+/2a0b10eb79e78d3aee52538d66768381769469dc
Patch Set 1 #
Total comments: 4
Patch Set 2 : comments & added histogram #
Total comments: 6
Patch Set 3 : crazy mojo mocking #Patch Set 4 : comments and removed tests #
Messages
Total messages: 21 (12 generated)
dmurph@chromium.org changed reviewers: + cmumford@chromium.org, jsbell@chromium.org, pwnall@chromium.org
Adding tests TODO (gonna copy over tests from another patch to make a framework) Can you PTAL for sanity? I'm guessing that canceling both the callbacks and the transaction is necessary.
Description was changed from ========== [IndexedDB] Abort transaction on unknown blob BUG=677780 ========== to ========== [IndexedDB] Abort transaction on unknown blobs instead of crashing There is a current refcounting known issue where a blob can be sent to another process but while that IPC is scheduled the source process dies. The blob will be non-existent. We want to gracefully handle this error in IndexedDB instead of crashing. BUG=677780 ==========
Description was changed from ========== [IndexedDB] Abort transaction on unknown blobs instead of crashing There is a current refcounting known issue where a blob can be sent to another process but while that IPC is scheduled the source process dies. The blob will be non-existent. We want to gracefully handle this error in IndexedDB instead of crashing. BUG=677780 ========== to ========== [IndexedDB] Abort transaction on unknown blobs instead of crashing There is a current refcounting known issue where a blob can be sent to another process but while that IPC is scheduled the source process dies. The blob will be non-existent. We want to gracefully handle this error in IndexedDB instead of crashing. BUG=677780 ==========
On 2017/03/27 20:33:10, dmurph wrote: > Adding tests TODO (gonna copy over tests from another patch to make a framework) > > Can you PTAL for sanity? I'm guessing that canceling both the callbacks and the > transaction is necessary. The code lgtm, assuming the assumption above is true. jsbell: Do you happen to remember how this works? If not, I can dig into the code.
https://codereview.chromium.org/2779433002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/database_impl.cc (right): https://codereview.chromium.org/2779433002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/database_impl.cc:278: UMA_HISTOGRAM_BOOLEAN("Storage.IndexedDB.PutValidBlob", !!handle); Can you please add a comment explaining why this is expected to happen? You can probably reuse most of the text in the commit message. https://codereview.chromium.org/2779433002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2779433002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_connection.cc:11: #include "content/browser/indexed_db/indexed_db_database_error.h" why?
dmurph@chromium.org changed reviewers: + mpearson@chromium.org
+mpearson for histograms.xml change. https://codereview.chromium.org/2779433002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/database_impl.cc (right): https://codereview.chromium.org/2779433002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/database_impl.cc:278: UMA_HISTOGRAM_BOOLEAN("Storage.IndexedDB.PutValidBlob", !!handle); On 2017/03/27 20:56:33, pwnall wrote: > Can you please add a comment explaining why this is expected to happen? You can > probably reuse most of the text in the commit message. Done. https://codereview.chromium.org/2779433002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_connection.cc (right): https://codereview.chromium.org/2779433002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_connection.cc:11: #include "content/browser/indexed_db/indexed_db_database_error.h" On 2017/03/27 20:56:33, pwnall wrote: > why? Missing include - required transitive includes.
Description was changed from ========== [IndexedDB] Abort transaction on unknown blobs instead of crashing There is a current refcounting known issue where a blob can be sent to another process but while that IPC is scheduled the source process dies. The blob will be non-existent. We want to gracefully handle this error in IndexedDB instead of crashing. BUG=677780 ========== to ========== [IndexedDB] Abort transaction on unknown blobs instead of crashing There is a current refcounting known issue where a blob can be sent to another process but while that IPC is scheduled the source process dies. The blob will be non-existent. We want to gracefully handle this error in IndexedDB instead of crashing. (also fixes missing include in the connection class) BUG=677780 ==========
histograms.ml lgtm modulo comments --mark https://codereview.chromium.org/2779433002/diff/20001/content/browser/indexed... File content/browser/indexed_db/database_impl.cc (right): https://codereview.chromium.org/2779433002/diff/20001/content/browser/indexed... content/browser/indexed_db/database_impl.cc:280: UMA_HISTOGRAM_BOOLEAN("Storage.IndexedDB.PutValidBlob", !!handle); nit: !!handle looks ugly to me. Can you rewrite it in something more easily interpretable? https://codereview.chromium.org/2779433002/diff/20001/content/browser/indexed... content/browser/indexed_db/database_impl.cc:292: UMA_HISTOGRAM_COUNTS("Storage.IndexedDB.PutBlobSizeKB", Why not UMA_HISTOGRAM_MEMORY_KB?
lgtm % nit https://codereview.chromium.org/2779433002/diff/20001/content/browser/indexed... File content/browser/indexed_db/database_impl.cc (right): https://codereview.chromium.org/2779433002/diff/20001/content/browser/indexed... content/browser/indexed_db/database_impl.cc:282: IndexedDBDatabaseError error(blink::WebIDBDatabaseExceptionUnknownError, We do want to go back to using mojo::ReportBadMessage once crbug.com/351753 is fixed right? If so can you add a TODO above, or a comment to that bug so that we don't forget?
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Since mocking out mojo stuff is taking so long / so complex, I'm splitting that out into another CL. https://codereview.chromium.org/2779433002/diff/20001/content/browser/indexed... File content/browser/indexed_db/database_impl.cc (right): https://codereview.chromium.org/2779433002/diff/20001/content/browser/indexed... content/browser/indexed_db/database_impl.cc:280: UMA_HISTOGRAM_BOOLEAN("Storage.IndexedDB.PutValidBlob", !!handle); On 2017/03/28 18:17:07, Mark P wrote: > nit: !!handle looks ugly to me. Can you rewrite it in something more easily > interpretable? Done. https://codereview.chromium.org/2779433002/diff/20001/content/browser/indexed... content/browser/indexed_db/database_impl.cc:282: IndexedDBDatabaseError error(blink::WebIDBDatabaseExceptionUnknownError, On 2017/03/28 18:49:31, cmumford wrote: > We do want to go back to using mojo::ReportBadMessage once crbug.com/351753 is > fixed right? If so can you add a TODO above, or a comment to that bug so that we > don't forget? Done. https://codereview.chromium.org/2779433002/diff/20001/content/browser/indexed... content/browser/indexed_db/database_impl.cc:292: UMA_HISTOGRAM_COUNTS("Storage.IndexedDB.PutBlobSizeKB", On 2017/03/28 18:17:07, Mark P wrote: > Why not UMA_HISTOGRAM_MEMORY_KB? sgtm
Dry run: 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 dmurph@chromium.org
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pwnall@chromium.org, mpearson@chromium.org, cmumford@chromium.org Link to the patchset: https://codereview.chromium.org/2779433002/#ps60001 (title: "comments and removed tests")
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": 60001, "attempt_start_ts": 1490811768973320,
"parent_rev": "12d7fecd789f8d242a3e8d1c71446d553e67b3e3", "commit_rev":
"2a0b10eb79e78d3aee52538d66768381769469dc"}
Message was sent while issue was closed.
Description was changed from ========== [IndexedDB] Abort transaction on unknown blobs instead of crashing There is a current refcounting known issue where a blob can be sent to another process but while that IPC is scheduled the source process dies. The blob will be non-existent. We want to gracefully handle this error in IndexedDB instead of crashing. (also fixes missing include in the connection class) BUG=677780 ========== to ========== [IndexedDB] Abort transaction on unknown blobs instead of crashing There is a current refcounting known issue where a blob can be sent to another process but while that IPC is scheduled the source process dies. The blob will be non-existent. We want to gracefully handle this error in IndexedDB instead of crashing. (also fixes missing include in the connection class) BUG=677780 Review-Url: https://codereview.chromium.org/2779433002 Cr-Commit-Position: refs/heads/master@{#460491} Committed: https://chromium.googlesource.com/chromium/src/+/2a0b10eb79e78d3aee52538d6676... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/2a0b10eb79e78d3aee52538d6676... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
