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)

Product:
Toolkit
Component:
Storage
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(1 file)

Bug 1422383 - Clone temporary tables, views, and triggers when cloning a storage connection.
59 bytes, text/x-review-board-request
mak
: review+
Details 
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 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 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 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+
Blocks: 1305563
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
(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.
https://hg.mozilla.org/mozilla-central/rev/cc8eaa26f2ed
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1423820
You need to log in before you can comment on or make changes to this bug.