STR: 1. Download (or find on your harddrive) a random jar/zip file. Let "file:///foo/bar.zip" be the path where it is saved. 2. In the location bar, type and hit enter on: jar:file///foo/bar.zip!/ Expected result: A listing of the files and directories in the jar/zip file. Actual result: An empty list. The last working nightly is: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20090729 Minefield/3.6a1pre Built from http://hg.mozilla.org/mozilla-central/rev/55fd6820c596 The first non-working nightly is: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.2a1pre) Gecko/20090730 Minefield/3.6a1pre Built from http://hg.mozilla.org/mozilla-central/rev/36a36a9c942b Looking at http://hg.mozilla.org/mozilla-central/shortlog/36a36a9c942b, the obvious culprit is bug 332173. I can't see the bug and don't have the time to decipher the patch to figure out why this broke. However, no .idl files changed, so I presume this is all still supposed to work, and that it not working is a bug... I would set this bug as depending on bug 332173, but I don't have the privileges to do so.
IINM, The trunk is running different code than the patch I wrote. It's a templatized derivative of my patch produced by Jeff Walden, so I'm adding him to the cc list here.
> 2. In the location bar, type and hit enter on: jar:file///foo/bar.zip!/ IINM, the correct syntax would be jar:file:///foo/bar.zip!/ ^ I am able to observe this empty listing issue on Windows with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090817 Namoroka/3.6a2pre and a URL such as jar:file:///c:/tmp/sslTraces.zip!/ which produces a non-empty listing in older browsers.
Interestingly, this is just a problem with the directory listing, not with accessing files in the jar's contents. For example, the URL jar:file:///c:/tmp/sslTraces.zip!/sslTraceDES.txt shows the contents of that file, even though jar:file:///c:/tmp/sslTraces.zip!/ shows an empty index.
(In reply to comment #2) > > 2. In the location bar, type and hit enter on: jar:file///foo/bar.zip!/ > IINM, the correct syntax would be jar:file:///foo/bar.zip!/ > ^ Uhm, yes, d'oh. I forgot a colon. (In reply to comment #3) > Interestingly, this is just a problem with the directory listing, not with > accessing files in the jar's contents. For example, the URL > jar:file:///c:/tmp/sslTraces.zip!/sslTraceDES.txt > shows the contents of that file, even though > jar:file:///c:/tmp/sslTraces.zip!/ > shows an empty index. Right. I actually discovered the issue because a user reported that Chrome List was broken on trunk (http://code.google.com/p/chromelist/issues/detail?id=39). There it was clear that using findEntries to find entries on a per-directory basis somehow no longer worked. I then found the regression range, and then realized that this probably broke jar directory browsing - and it had! :-)
At http://mxr.mozilla.org/mozilla-central/source/modules/libjar/nsJARInputStream.cpp#119 we read: 119 // We can get aDir's contents as strings via FindEntries 120 // with the following pattern (see nsIZipReader.findEntries docs) 121 // assuming dirName is properly escaped: 122 // 123 // dirName + "?*~" + dirName + "?*/?*" The pattern being passed to ns_WildCardMatch<char> is "?*~?*/?*" and the file names (in my test case) are simple file names. The return value is the NOMATCH value. This is unexpected. I wonder if the author mistakenly (but understandably) thought that the function takes patterns that are regular expressions rather than file name globbing expressions. But regardless, that pattern should work. Unfortunately, the debugger is thoroughly confused by the templatized code, and is unable to step through it, or even correctly disassemble it. So, I think I will try walking through the non-templatized version of the code to see how it behaves.
I found the bug. IINM, this bug is only in the templatized derivative of my patch, and not in my original code. The bug only occurs with patterns that contain the '~' character. My original code looks like this: 358 if (!strchr(xp, '~')) 359 return _shexp_match(str, xp, case_insensitive, 0); 360 361 exp = PORT_Strdup(xp); 362 if(!exp) 363 return NOMATCH; 364 365 x = _scan_and_copy(exp, '~', '\0', NULL); The templatized code translated that, in static function ns_WildCardMatch, as 421 if (!nsCharTraits<T>::find(xp, nsCharTraits<T>::length(xp), T('~'))) 422 return _shexp_match(str, xp, case_insensitive, 0); 423 424 expr = (T *) NS_Alloc((nsCharTraits<T>::length(xp) + 1) * sizeof(T)); 425 if(!expr) 426 return NOMATCH; 427 428 x = ::_scan_and_copy(expr, T('~'), T('\0'), static_cast<T*>(NULL)); The problem (IINM) is that the NS_Alloc at line 424 does not duplicate the functionality of the original PORT_Strdup call. It does not fill the newly allocated buffer with a copy of the original pattern string "xp". That's the problem.
Assignee: nobody → jwalden+bmo
Oh gag. I hate this code. Rubber-stamp r=me to add this line to fix the problem, if you can whip up the patch (I'll push if you want), which probably should go in branches as well: memcpy(expr, xp, (nsCharTraits<T>::length(xp) + 1) * sizeof(T));
isn't this bug a dupe of 509267
(In reply to comment #8) > isn't this bug a dupe of 509267 Yes and no. The jar directory browsing bit is a dupe. The findEntries bit isn't; the last few comments in bug 509267 seem to indicate it might be fixed by no longer using findEntries.
I think this should block 1.9.2 because it is a serious regression for all consumers of the nsIZipReader API. There are visible effects in at least Firefox (on the jar directory browsing, see bug 509267, with a potential other way to fix that specific issue), Instantbird (https://bugzilla.instantbird.org/show_bug.cgi?id=211) and Chrome List (http://code.google.com/p/chromelist/issues/detail?id=39). If I understand comment 6 and comment 7 correctly, the fix is trivial.
Sorry, I cannot work on this bug any more at this time.
OK, so this is, afaict, what comment #7 meant. I patched against 1.9.2 as I'm on OS X 10.4, and trunk no longer compiles on that, to my knowledge. It works on my box, and I verified it fixes Chrome List's issues. I put it up on try server (testing branch - which in hindsight perhaps wasn't the cleverest idea) as I figured changing core stuff might cause issues... Does this need SR? I don't *think* so, but I could be wrong, as the rules changed semi-recently and I've never patched anything in XPCOM (or if I did it was a very long time ago). :-)
Assignee: jwalden+bmo → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #402670 - Flags: review?
Attachment #402670 - Flags: review? → review?(jwalden+bmo)
(In reply to comment #12) > I put it up on try > server (testing branch - which in hindsight perhaps wasn't the cleverest idea) > as I figured changing core stuff might cause issues... Builds seem to have gone OK - except two of the unit test boxen are failing the same getBoundingClientRect tests, there's a random-looking reftest failure, and some test timeouts... I'm presuming that has nothing to do with this patch, but maybe I'm wrong? Logs: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1253828343.1253834486.18946.gz http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1253828083.1253834064.14394.gz http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1253828083.1253833776.11308.gz That, and MacOSX Darwin 8.8.1 try talos dirty is very unhappy, but that seems to be the case for all the tryserver builds, so I'm 99% sure that isn't the patch's fault...
Attachment #402670 - Flags: review?(jwalden+bmo) → review+
Gijs, you should land this please. Did this also regress on 1.9.0 and 1.9.1?
blocking1.9.1: --- → ?
Is there a particular reason we can't use NS_strdup or nsMemory::Clone?
On the 1.9.0 and 1.9.1 branches the libjar copy of nsWildcard is fine, but this fix is needed in the xpfe/components/filepicker copy.
(In reply to comment #15) > Is there a particular reason we can't use NS_strdup or nsMemory::Clone? I don't know. Is there an advantage to doing so? If so, please submit the alternate patch for review... I am not at all an expert in this area of the code, the bug simply bit me and I was the first to put up a patch that I didn't even come up with myself... (In reply to comment #14) > Gijs, you should land this please. Apologies; I have been in transit and adjusting to EST (where I am sort of on holiday). At the moment the tree is red. Barring a decision to use an alternate approach (comment #15), if someone gets to it before me, feel free to check in. If that doesn't happen, I will try to get to it tonight, and otherwise tomorrow morning (EST).
(In reply to comment #17) > (In reply to comment #14) > > Gijs, you should land this please. > > Apologies; I have been in transit and adjusting to EST (where I am sort of on > holiday). At the moment the tree is red. Barring a decision to use an alternate > approach (comment #15), if someone gets to it before me, feel free to check in. > If that doesn't happen, I will try to get to it tonight, and otherwise tomorrow > morning (EST). http://hg.mozilla.org/mozilla-central/rev/3a091a5c93d9
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Oops, 1.9.2 checkin was: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/f1e55c249e20
Comment on attachment 404038 [details] [diff] [review] Patch for 1.9.0 filepicker copy Carrying over review, requesting approval per comment #16.
Comment on attachment 404038 [details] [diff] [review] Patch for 1.9.0 filepicker copy Approved for 184.108.40.206 and 220.127.116.11, a=dveditz for release-drivers
Checking in mozilla/xpfe/components/filepicker/src/nsWildCard.cpp; /cvsroot/mozilla/xpfe/components/filepicker/src/nsWildCard.cpp,v <-- nsWildCard.cpp new revision: 1.5; previous revision: 1.4 done
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fb0c1de78c61 Unless I missed something, we're done in this bug...
Looks like it to me, too. Thank-you!
Whiteboard: zipreader OK on 1.9.0/1.9.1, affects filepicker everywhere 332173 landed
You need to log in before you can comment on or make changes to this bug.