Copy temp entities after reattaching databases to a cloned connection

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lina, Assigned: lina)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment)

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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.