1274585 - Push HTTP cache index build when asked for disk cache size

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 2

[Michal Novotny [:michal]]

Comment on attachment 8754842 [details] [diff] [review]
v1

This seems to be a wrong patch.

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v1

This is the one.

Comment 4

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1c9de554922cbf6f3f1f7f8e7de9e0f3081963e

Comment 5

[Michal Novotny [:michal]]

Comment on attachment 8755110 [details] [diff] [review]
v1

Review of attachment 8755110 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheIndex.cpp
@@ +1372,5 @@
> +  // Move forward with index re/building if it is pending
> +  RefPtr<CacheIOThread> ioThread = CacheFileIOManager::IOThread();
> +  if (ioThread) {
> +    ioThread->Dispatch(NS_NewRunnableFunction([]() -> void {
> +      RefPtr<CacheIndex> index = gInstance;

gInstance must be accessed under sLock

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Comment 7

[Michal Novotny [:michal]]

Comment on attachment 8756364 [details] [diff] [review]
v1.1

Review of attachment 8756364 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheIndex.cpp
@@ +1378,5 @@
> +      RefPtr<CacheIndex> index = gInstance;
> +      if (index && index->mUpdateTimer) {
> +        index->mUpdateTimer->Cancel();
> +        index->mUpdateTimer = nullptr;
> +        index->DelayedUpdate(nullptr, nullptr);

DelayedUpdate() grabs the lock too, so you have to call it outside the lock.

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v1.2

Comment 9

[Michal Novotny [:michal]]

Comment on attachment 8757370 [details] [diff] [review]
v1.2

Review of attachment 8757370 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheIndex.cpp
@@ +2517,5 @@
> +}
> +
> +// static
> +void
> +CacheIndex::DelayedUpdateUnlocked()

If think DelayedUpdateLocked makes IMO more sense.

@@ +2523,1 @@
>    LOG(("CacheIndex::DelayedUpdate()"));

Please update logged names too.

@@ +2529,5 @@
>    RefPtr<CacheIndex> index = gInstance;
>  
>    if (!index) {
>      return;
>    }

Could you make DelayedUpdateLocked non-static method and check gInstance in DelayedUpdate?

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1.3

Comment 11

[Michal Novotny [:michal]]

Comment on attachment 8757890 [details] [diff] [review]
v1.3

Review of attachment 8757890 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!

Comment 12

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eba918466e7

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6750817f09e1

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6750817f09e1