967941 - Convert nsZipReaderCache::mZips to a modern hashtable

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Comment 1

[Andrew McCreight (out of office until 8/21) [:mccr8]]

As a bonus, I can fix the quadratic behavior in nsZipReaderCache::Observe.

Comment 2

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 1 - Remove some trailing whitespace from nsJAR.{h,cpp}.

Comment 3

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 2 - Convert nsZipReaderCache::mZips to use nsRefPtrHashtable.

With this patch, modules/libjar/test/unit/test_bug637286.js is failing.  It hits some kind of deadlock detection.  My theory is that this is due to causing a change in how objects are addrefed and released, causing some object to die too early, but I haven't had any success investigating it so far.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 1 - Remove some trailing whitespace from nsJAR.{h,cpp}.

Taras, let me know if you aren't the right person to review this.  I could get froydnj to review it instead or something.

https://tbpl.mozilla.org/?tree=Try&rev=919977e5c90b

Comment 5

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 2 - Convert nsZipReaderCache::mZips to use nsRefPtrHashtable.

Comment 6

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Attachment: part 3 - Smart pointerize nsZipReaderCache methods.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8379771 [details] [diff] [review]
part 1 - Remove some trailing whitespace from nsJAR.{h,cpp}.

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

::: modules/libjar/nsJAR.cpp
@@ +121,5 @@
>  #endif
>        mCache->ReleaseZip(this);
>      NS_ASSERTION(NS_SUCCEEDED(rv), "failed to release zip file");
>    }
>    return count; 

There's a trailing space here.

@@ +639,2 @@
>      curLine.Assign(curPos, linelen);
>      if (linelen == 0) 

There's a trailing space here.

@@ +1037,5 @@
>  
>  NS_IMETHODIMP
>  nsZipReaderCache::Init(uint32_t cacheSize)
>  {
>    mCacheSize = cacheSize; 

and here

@@ +1044,1 @@
>    nsCOMPtr<nsIObserverService> os = 

and here

@@ +1209,5 @@
>    }
>    return true;
>  }
>  
>  struct ZipFindData {nsJAR* zip; bool found;}; 

Here

::: modules/libjar/nsJAR.h
@@ +144,2 @@
>      uint32_t     mPermissions;
>      bool mIsDirectory; 

There's a trailing space here.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8379773 [details] [diff] [review]
part 2 - Convert nsZipReaderCache::mZips to use nsRefPtrHashtable.

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

::: modules/libjar/nsJAR.cpp
@@ +1279,2 @@
>    nsRefPtr<nsJAR> removed;
> +  mZips.Remove(uri, getter_AddRefs(removed));

I can't find any overload of Remove() that does a Get + Remove in one call.

Comment 9

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Ah, yeah, sorry, I'm adding one in bug 975223.  I was just going to wait on this patch being ready to land before landing it.

Comment 10

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Comment on attachment 8379773 [details] [diff] [review]
part 2 - Convert nsZipReaderCache::mZips to use nsRefPtrHashtable.

See comment 9.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8379773 [details] [diff] [review]
part 2 - Convert nsZipReaderCache::mZips to use nsRefPtrHashtable.

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

Cool.

Comment 12

[Andrew McCreight (out of office until 8/21) [:mccr8]]

Thanks for the reviews!  I just went ahead and removed all trailing whitespace in nsJAR.h and nsJAR.cpp.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/36e79221886d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ef74212f2e25
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/68c714066b0c

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36e79221886d
https://hg.mozilla.org/mozilla-central/rev/ef74212f2e25
https://hg.mozilla.org/mozilla-central/rev/68c714066b0c