24765 - use cache manager for downloaded jar files

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect, P2)

Description

[Warren Harris]

Currently downloaded jar files end up in a subdirectory under the current 
process directory. We need to use the facilities in the cache manager to save 
jar files to that proper space accounting and cleanup is done.

Comment 1

[Scott Furman]

Warren, I suggest you mark this bug a duplicate of 21250.

Comment 2

[Warren Harris]

I'd say this one depends on 21250. Once we have stream-as-file, we need to hook 
up the jar protocol to use it. Let's keep both of these.

Comment 3

[Phil Peterson]

PDT+

Comment 4

[Warren Harris]

I'm not sure why this was marked PDT+. I think it's perfectly fine if the cache 
doesn't manage downloaded jar files right now. Yes, they'll pile up and won't 
get flushed, but that seems ok for beta.

Removing PDT+ designation from whiteboard in hopes that they'll read this and 
reaccess. Probably was just a mistake that I put beta1 in the keywords.

Comment 5

[gayatrib]

We should adjust 21250 based on the status of this bug. Bug 21250 is currently 
not a beta1 candidate.

Comment 6

[Warren Harris]

Moving to M15.

Comment 7

[Warren Harris]

Marking beta2.

Comment 8

[Warren Harris]

M16

Comment 9

[Warren Harris]

Moving to M17 which is now considered part of beta2.

Comment 10

[leger]

Putting on [nsbeta2-] radar. Not critical to beta2.  Spoke with warren, he is 
cool with this.

Comment 11

[Warren Harris]

A must for nsbeta3. We're currently eating up the user's disk space and never 
freeing it. We all know how this was a customer escalation in 3.x.

Comment 12

[Warren Harris]

*** Bug 48536 has been marked as a duplicate of this bug. ***

Comment 13

[Mitchell Stoltz (not reading bugmail)]

Reassigning to me.

Comment 14

[Mitchell Stoltz (not reading bugmail)]

I'm attaching a patch plus three new files which should be installed as
netwerk/base/public/nsIDownloader.idl, base/src/nsDownloader.h, and
base/src/nsDownloader.cpp. I've tested this for several remote JAR URLs and some
jar:file URLs as well, it seems to work in both cases. All of my testcases
called AsyncRead() on the JARChannel; I can't find a case which tests
OpenInputStream() so I'm not sure if this works. Do you know how to trigger this
case, Warren?

Subsequent reloads of the same page seem to be hitting the cache, not the
network. I can't really tell how good it is about updating when the remote file
changes, but it seems to do roughly as well as ordinary http pages. jar:ftp:
does not work since ftp does not support stream-as-file caching, but that's not
a high priority. I've also made signature verification lazy and added some code
to nsJARURI so that GetHost() and similar calls will work for JAR URLs.

Comment 15

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch, proposed fix.

Comment 16

[Mitchell Stoltz (not reading bugmail)]

Attachment: New file, mozilla/netwerk/base/public/nsIDownloader.idl. Part of patch.

Comment 17

[Mitchell Stoltz (not reading bugmail)]

Attachment: mozilla/netwerk/base/src/nsDownloader.h, part of patch

Comment 18

[Mitchell Stoltz (not reading bugmail)]

Attachment: mozilla/netwerk/base/src/nsDownloader.cpp, part of patch

Comment 19

[Warren Harris]

I tried the patch and it doesn't work. Nothing comes up at all.

Sorry, but I don't have time to trace through it before I leave. I'll enclose 
my updated diffs though.

Comment 20

[Warren Harris]

Attachment: revised patch -- still doesn't work

Comment 21

[Warren Harris]

BTW, Mitch, the changes you made to nsJARURI aren't right (I noticed them in my 
revised patch). Jar URLs don't have any concept of a 
host/port/username/password/prehost or path. Those methods need to fail.

I suspect you implemented them because you found that they were called and 
caused something else to fail. Let's find the place that's trying to access 
them and see what it's trying to do.

Comment 22

[Mitchell Stoltz (not reading bugmail)]

Attachment: Yet another patch - mostly works

Comment 23

[Mitchell Stoltz (not reading bugmail)]

Upping priority since the jar protocol and signed scripts are of limited 
usefulness without this.

Comment 24

[Mitchell Stoltz (not reading bugmail)]

Attachment: Fix for jar packaging on Windows

Comment 25

[Phil Peterson]

PDT thinks this is P2.

Comment 26

[Brendan Eich [:brendan]]

diff -u format please, I am not going to review diff -c with bogus indentation.

There appear to be tab and indentation problems and variations (the in boolean
synchronous parameter to init in nsIDownLoader.idl, the mixing of four-space
with two-space indentation in nsDownLoader.cpp, etc.).  For example,

!   if (mObserver && (NS_FAILED(rv) || localFile)) 
!   {

There is an else-after-return non-sequitur, two-space rather than four-space
equivalent indentation, and more brace-style conflict with the prevailing style
used in the rest of the file on the subsequent lines:

!      if (aSynchronous)
!        return mObserver->OnDownloadComplete(this, mContext, rv, localFile);
!      else
!      {
!        // If the open failed or the file is local, call the observer.
!        // don't callback synchronously as it puts the caller
!        // in a recursive situation and breaks the asynchronous
!        // semantics of nsIDownloader

!        nsresult rv2 = NS_OK;
!        NS_WITH_SERVICE(nsIProxyObjectManager, pIProxyObjectManager, 
!                        kProxyObjectManagerCID, &rv);
!        if (NS_FAILED(rv2)) return rv2;
!            nsCOMPtr<nsIDownloadObserver> pObserver;

We just indented four spaces for no reason -- hard tabs in file?

!            rv2 = pIProxyObjectManager->GetProxyForObject(NS_CURRENT_EVENTQ, 
!                      NS_GET_IID(nsIDownloadObserver), mObserver, 
!                      PROXY_ASYNC | PROXY_ALWAYS, getter_AddRefs(pObserver));
!            if (NS_FAILED(rv2)) return rv2;
!            return pObserver->OnDownloadComplete(this, mContext, rv,
localFile);
!      }

It's not good manners to go against the prevailing style used in netwerk code,
but if you own this file (it looks like you checked in the first revision) and
want to assert your own style, please make it self-consistent style throughout.

/be

Comment 27

[Mitchell Stoltz (not reading bugmail)]

Attachment: OK, patch number five, fixes leak. Review please.

Comment 28

[Mitchell Stoltz (not reading bugmail)]

By Jove, I think we've got it. Fix checked in, finally. We should no longer be 
creating a bin/components/jarCache directory, and files loaded via the jar 
protocol from an http source should stay up to date according to the standard 
file cache policy.

Comment 29

[Tom Everingham]

verified.

Comment 30

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/0bf3bd07ca66
[fenix] Fix for https://github.com/mozilla-mobile/fenix/issues/24765: Update to JNA 5.8.0