Closed Bug 1423820 Opened 3 years ago Closed 3 years ago

Copy temp entities after reattaching databases to a cloned connection

Categories

(Toolkit :: Storage, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

Attachments

(1 file)

We should copy over temp entities after reattaching databases, in case the connection sets up temp triggers on attached tables. Places does this for `moz_icons` in https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/toolkit/components/places/nsPlacesTriggers.h#297-302.

We should also probably close the cloned connection if copying temp entities fails, though I'm not sure if the `AsyncInitializeClone` machinery already handles that for us when it releases `mClone`.
Comment on attachment 8935250 [details]
Bug 1423820 - Copy temp entities after reattaching databases to a cloned storage connection.

https://reviewboard.mozilla.org/r/206134/#review211728


C/C++ static analysis found 4 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: storage/mozStorageConnection.cpp:1536
(Diff revision 1)
> +  });
> +
> +  // Re-attach on-disk databases that were attached to the original connection.
> +  {
> +    nsCOMPtr<mozIStorageStatement> stmt;
> +    rv = CreateStatement(NS_LITERAL_CSTRING("PRAGMA database_list"),

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

::: storage/mozStorageConnection.cpp:1549
(Diff revision 1)
> +                              !name.EqualsLiteral("temp")) {
> +        nsCString path;
> +        rv = stmt->GetUTF8String(2, path);
> +        if (NS_SUCCEEDED(rv) && !path.IsEmpty()) {
> +          nsCOMPtr<mozIStorageStatement> attachStmt;
> +          rv = aClone->CreateStatement(

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

::: storage/mozStorageConnection.cpp:1553
(Diff revision 1)
> +          nsCOMPtr<mozIStorageStatement> attachStmt;
> +          rv = aClone->CreateStatement(
> +            NS_LITERAL_CSTRING("ATTACH DATABASE :path AS ") + name,
> +            getter_AddRefs(attachStmt));
> +          MOZ_ASSERT(NS_SUCCEEDED(rv));
> +          rv = attachStmt->BindUTF8StringByName(NS_LITERAL_CSTRING("path"),

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

::: storage/mozStorageConnection.cpp:1556
(Diff revision 1)
> +            getter_AddRefs(attachStmt));
> +          MOZ_ASSERT(NS_SUCCEEDED(rv));
> +          rv = attachStmt->BindUTF8StringByName(NS_LITERAL_CSTRING("path"),
> +                                                path);
> +          MOZ_ASSERT(NS_SUCCEEDED(rv));
> +          rv = attachStmt->Execute();

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
Actually, we need to do this after setting `PRAGMA temp_store`, because https://sqlite.org/pragma.html says:

> When the temp_store setting is changed, all existing temporary tables, indices, triggers, and views are immediately deleted.

I also wrapped this up in a transaction, but I don't know if that's strictly necessary.
Comment on attachment 8935250 [details]
Bug 1423820 - Copy temp entities after reattaching databases to a cloned storage connection.

https://reviewboard.mozilla.org/r/206134/#review213100
Attachment #8935250 - Flags: review?(mak77) → review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1f8af3283ae
Copy temp entities after reattaching databases to a cloned storage connection. r=mak
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/d1f8af3283ae
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.