Closed
Bug 1422383
Opened 3 years ago
Closed 3 years ago
Cloning a connection should also clone its temp tables and triggers
Categories
(Toolkit :: Storage, defect)
Toolkit
Storage
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: lina, Assigned: lina)
References
Details
Attachments
(1 file)
Context: in bug 1305563, I'm working on a buffer and merger for synced bookmarks. Merging attaches the buffer connection to Places, starts an exclusive transaction, updates Places, and commits. I'm using a separate connection because the main Places connection has logic that detects if a transaction is already in progress, and continues anyway if one is already active. If I used the main connection, the buffer's changes might interleave with writes from `Bookmarks.jsm` and the synchronous `PlacesUtils.bookmarks` methods, causing coherency issues when the merger tries to apply the merged tree back to Places. However, using a separate connection will also cause issues, as Mak noted in bug 1305563, comment 163, because it doesn't have any of the Places temp tables or triggers. Bug 1305563, comment 165 and bug 1305563, comment 168 have more discussion. One possible fix is to have `clone` copy over temp entities when it clones a connection. That way, the merger can clone the Places connection, attach to the buffer, and update Places.
Comment hidden (mozreview-request) |
Comment 2•3 years ago
|
||
mozreview-review |
Comment on attachment 8933743 [details] Bug 1422383 - Clone temporary tables, views, and triggers when cloning a storage connection. https://reviewboard.mozilla.org/r/204682/#review210260 C/C++ static analysis found 2 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: storage/mozStorageConnection.cpp:1534 (Diff revision 1) > + // Copy over temporary tables, triggers, and views from the original > + // connections. Entities in `sqlite_temp_master` are only visible to the > + // connection that created them. > + if (!aReadOnly) { > + nsCOMPtr<mozIStorageStatement> stmt; > + rv = CreateStatement(NS_LITERAL_CSTRING("SELECT sql FROM sqlite_temp_master"), Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores] ::: storage/mozStorageConnection.cpp:1551 (Diff revision 1) > + if (StringBeginsWith(query, NS_LITERAL_CSTRING("CREATE TABLE ")) || > + StringBeginsWith(query, NS_LITERAL_CSTRING("CREATE TRIGGER ")) || > + StringBeginsWith(query, NS_LITERAL_CSTRING("CREATE VIEW "))) { > + query.Replace(0, 6, "CREATE TEMP"); > + } > + rv = aClone->ExecuteSimpleSQL(query); Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
Comment 3•3 years ago
|
||
mozreview-review |
Comment on attachment 8933743 [details] Bug 1422383 - Clone temporary tables, views, and triggers when cloning a storage connection. https://reviewboard.mozilla.org/r/204682/#review210306 This seems fine to me, and I'll leave the review to :mak. We should also update the documentation on mozIStorageConnection::clone[1] and mozIStorageAsyncConnection::asyncClone[2], emphasizing we're only propagating the temporary schema and none of its data. 1: https://searchfox.org/mozilla-central/source/storage/mozIStorageConnection.idl#59 2: https://searchfox.org/mozilla-central/source/storage/mozIStorageAsyncConnection.idl#62
Attachment #8933743 -
Flags: review?(bugmail)
Comment hidden (mozreview-request) |
Comment 5•3 years ago
|
||
mozreview-review |
Comment on attachment 8933743 [details] Bug 1422383 - Clone temporary tables, views, and triggers when cloning a storage connection. https://reviewboard.mozilla.org/r/204682/#review210602 ::: storage/mozStorageConnection.cpp:1535 (Diff revision 2) > + // connections. Entities in `sqlite_temp_master` are only visible to the > + // connection that created them. > + if (!aReadOnly) { > + nsCOMPtr<mozIStorageStatement> stmt; > + rv = CreateStatement(NS_LITERAL_CSTRING("SELECT sql FROM sqlite_temp_master"), > + getter_AddRefs(stmt)); just to be on the safe side, I'd like to see a "WHERE type IN ('table', 'view', 'index', 'trigger') " ::: storage/test/unit/test_storage_connection.js:875 (Diff revision 2) > + onFunctionCall(args) { > + this.name = args.getUTF8String(0); > + }, > + }; > + let func = new storeLastInsertedNameFunc(); > + db.createFunction("store_last_inserted_name", 1, func); I wonder what happens if we createFunction in js and then asyncClone. asyncClone is a bit dangerous from this point of view, since it clones functions that could not be thread-safe. Not your fault clearly, but could be worth filing a bug to track that problem. We should at least make CreateFuntion [noscript]
Attachment #8933743 -
Flags: review?(mak77) → review+
Comment hidden (mozreview-request) |
Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cc8eaa26f2ed Clone temporary tables, views, and triggers when cloning a storage connection. r=mak
Assignee | ||
Comment 8•3 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #5) > I wonder what happens if we createFunction in js and then asyncClone. > asyncClone is a bit dangerous from this point of view, since it clones > functions that could not be thread-safe. I tried this the first time around, and just cloning a connection with a JS-implemented SQL function trips an XPConnect `AddRef` thread-safety assertion; you don't even need to call the function to crash. Bug 1392238 seems to cover this.
Comment 9•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cc8eaa26f2ed
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•