Open Bug 581616 Opened 12 years ago Updated 6 months ago

Allow zip files to be incrementally read

Categories

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

defect

Tracking

()

Tracking Status
blocking2.0 --- -

People

(Reporter: mwu, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [necko-would-take])

Attachments

(2 files, 2 obsolete files)

No description provided.
Blocks: 529208
Blocks: 582061
This is blocking resource packages which are blocking 2.0, so this should block too.
blocking2.0: --- → ?
Attached patch WIP (obsolete) — Splinter Review
Still need to write tests so we know if this code actually works.
Comment on attachment 462620 [details] [diff] [review]
WIP

Opps. Forgot to finish implementing the decompression part.
Attachment #462620 - Attachment is obsolete: true
blocking2.0: ? → betaN+
I saw that omnijar landed; that's awesome!  What's the status with this patch?
mwu, do you have an ETA for a working version of this patch?
mwu, the current WIP doesn't compile against trunk.  Do you have a newer patch, or should I rebase it myself?
(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.
Attached patch Add nsAsyncZipReader (WIP) (obsolete) — Splinter Review
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.
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
Ah, I'd forgotten to apply patch 5a from bug 533038.  Looks like it's compiling ok now!
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.
Attached patch Hacked patch v1Splinter Review
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.
Attachment #469311 - Attachment is obsolete: true
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
(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.
(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.
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.
blocking2.0: betaN+ → -
Blocks: 175008
Whiteboard: [necko-would-take]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
Assignee: mwu.code → nobody
You need to log in before you can comment on or make changes to this bug.