1423820 - Copy temp entities after reattaching databases to a cloned connection

Status: RESOLVED FIXED

(Toolkit :: Storage, enhancement, P1)

Description

[Lina Butler [:lina]]

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 1

[Lina Butler [:lina]]

Attachment: Bug 1423820 - Copy temp entities after reattaching databases to a cloned storage connection.

Review commit: https://reviewboard.mozilla.org/r/206134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206134/

Comment 2

[Code Review Bot [:reviewbot]]

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]

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8935250 [details]
Bug 1423820 - Copy temp entities after reattaching databases to a cloned storage connection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206134/diff/1-2/

Comment 4

[Lina Butler [:lina]]

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 5

[Lina Butler [:lina]]

Comment on attachment 8935250 [details]
Bug 1423820 - Copy temp entities after reattaching databases to a cloned storage connection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206134/diff/2-3/

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8935250 [details]
Bug 1423820 - Copy temp entities after reattaching databases to a cloned storage connection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206134/diff/3-4/

Comment 7

[Lina Butler [:lina]]

Comment on attachment 8935250 [details]
Bug 1423820 - Copy temp entities after reattaching databases to a cloned storage connection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206134/diff/4-5/

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1f8af3283ae
Copy temp entities after reattaching databases to a cloned storage connection. r=mak

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/d1f8af3283ae