581616 - Allow zip files to be incrementally read

Status: NEW

(Core :: Networking: JAR, defect, P5)

Comment 1

[Justin Lebar (not reading bugmail)]

This is blocking resource packages which are blocking 2.0, so this should block too.

Comment 2

[Michael Wu [:mwu]]

Attachment: WIP

Still need to write tests so we know if this code actually works.

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 462620 [details] [diff] [review]
WIP

Opps. Forgot to finish implementing the decompression part.

Comment 4

[Justin Lebar (not reading bugmail)]

I saw that omnijar landed; that's awesome!  What's the status with this patch?

Comment 5

[Justin Lebar (not reading bugmail)]

mwu, do you have an ETA for a working version of this patch?

Comment 6

[Justin Lebar (not reading bugmail)]

mwu, the current WIP doesn't compile against trunk.  Do you have a newer patch, or should I rebase it myself?

Comment 7

[Michael Wu [:mwu]]

(In reply to comment #6)
> mwu, the current WIP doesn't compile against trunk.  Do you have a newer patch,
> or should I rebase it myself?
I'll rebase.

Comment 8

[Michael Wu [:mwu]]

Attachment: Add nsAsyncZipReader (WIP)

I moved the async stuff to its own class. This patch depends on patch 5a in bug 533038. None of this code is tested yet, but all the problems I'm aware of are fixed with the exception of handling truncated zips. It probably still won't work.

Comment 9

[Justin Lebar (not reading bugmail)]

This still isn't compiling against tip of trunk (d906347e3a36):

In file included from ../../../src/modules/libjar/nsJAR.h:64,
                 from ../../../src/xpcom/components/nsManifestZIPLoader.cpp:43:
../../../src/modules/libjar/nsZipArchive.h: In member function ‘const PRUint8* nsZipItemPtr_base::GetData()’:
../../../src/modules/libjar/nsZipArchive.h:328: error: ‘mFileData’ was not declared in this scope
../../../src/modules/libjar/nsZipArchive.h: In member function ‘PRUint32 nsZipItemPtr_base::Length()’:
../../../src/modules/libjar/nsZipArchive.h:329: error: ‘mLen’ was not declared in this scope

Comment 10

[Justin Lebar (not reading bugmail)]

Ah, I'd forgotten to apply patch 5a from bug 533038.  Looks like it's compiling ok now!

Comment 11

[Justin Lebar (not reading bugmail)]

I've made some progress hooking this up to resource packages, but I'm still running into issues.

The latest problem I've hit is that nsJARInputStream::ReadBuffer seems to assume that DispatchCallback() synchronously notifies the input stream that the input stream is ready, while callback->OnInputStream is actually an asynchronous call.

I can (and may) continue hacking on this, but I thought it might be valuable at this point to have the original author look at what's going on here, since I'm not familiar e.g. with the threading model.  In a moment, I'll post my working incremental extraction and resource packages patches.

I think it would be really useful if we could write a test suite for the async JAR interface which doesn't involve resource packages.  That might make it easier to coordinate testing.  In the meantime, resource packages is blocked on these zip reader issues.

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Hacked patch v1

A lot of the changes I've made to this are hacks -- I'm just trying to get something to even kind of work right now.

Comment 13

[Justin Lebar (not reading bugmail)]

Attachment: Hacked resource packages v1

This applies on top of the async zip reader patch.

The test I've been working on is the very first reftest in netwerk/test/resourcepackage/reftests/reftest.list

Comment 14

[Michael Wu [:mwu]]

(In reply to comment #11)
> The latest problem I've hit is that nsJARInputStream::ReadBuffer seems to
> assume that DispatchCallback() synchronously notifies the input stream that the
> input stream is ready, while callback->OnInputStream is actually an
> asynchronous call.
> 
Shouldn't matter much. It breaks an optimization but should work by falling back on reading from the file. Of course, the file reading fallback code could be failing in this case.

Comment 15

[Justin Lebar (not reading bugmail)]

(In reply to comment #14)
> Shouldn't matter much. It breaks an optimization but should work by falling
> back on reading from the file. Of course, the file reading fallback code could
> be failing in this case.

Right.  The problem is that the data doesn't get written to the file until after the OnDataAvailable call.  So we fall back to reading from the file, which doesn't have the data (but which we assume does have it) and bad things happen.

Comment 16

[Jason Duell]

Resource packages aren't going to make it into 2.0, which is what this blocks,  so I don't think this should be an FF 4 blocker.

Comment 17

[Justin Lebar (not reading bugmail)]

Agreed.

Comment 18

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258