User-Agent: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.3a1pre) Gecko/20090815 Minefield/3.7a1pre Build Identifier: After a successful build 2 days ago, Minefield wouldn't start. With profilemanager I see in the window list of eCenter something popping up "Title" (I can close the task). In the center of my screen I see a small (2 cm) vertical line, that's all. After moving my profile dir out of the way to get a new profile, I see a crash in xul. Fortunately, not much was checked in after the last successful build 3 days ago. Therefore I could find the reason for the crash It was the checkin for bug 504864 "mmap io for JARs" http://hg.mozilla.org/mozilla-central/rev/ad1b7a04fbba After reverse patching my tree Minefield comes up again as usual. Reproducible: Always

OS/2 failure kind of suggests that NSPR doesn't do mmap correctly on that platform. assuming mmap works on os/2 should be an easy fix to nspr

One could say that this is a dupe of bug 417450. (I always thought we were using mmap for JARs and were doing a fallback for platforms that cannot do that, like OS/2.)

(In reply to comment #1) > assuming mmap works on os/2 should be an easy fix to nspr Sadly, there's no mmap(). A credible emulation could be created, but it would be subject to constaints that may not be possible in this context: - a data structure used by the OS has to be created on the stack by some top-level function; - all mapped i/o activity (init, read/write, close) has to be performed either by the top-level function or functions it calls to ensure the structure remains on the stack; - all mapped i/o activity _must_ occur on the same thread. Even if the constraints can be met in this case, I question whether they could be met in possible future uses of mmap. Overall, I think we'd be better off faking it. Instead of implementing i/o-on-demand, we should just read the relevant part of the file when it's mapped. It would be constraint-free and vastly simpler.

I agree, for now easiest solution is to malloc+ read in whole file. The whole file is relevant. I'll think about adding a more sophisticated fallback if I ever see gigantic addons.

Since a true mmap() emulation for NSPR isn't practical, this patch simulates it by reading the entire mapped segment of a file when PR_MemMap() is called. It supports ReadOnly and WriteCopy modes, but not ReadWrite because it has no way to map memory back to the original file.

Comment on attachment 396119 [details] [diff] [review] simulate memmap in NSPR Thanks Rich, looks good to me on a quick glance. I'll take another closer look over the next days. I think Wan-Teh should review this, too.

Fix will be in NSPR.

Taras: previously people implemented workaround in their own code when PR_CreateFileMap fails with PR_NOT_IMPLEMENTED_ERROR. Here is an example from NSS: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/legacydb/dbmshim.c&rev=1.2&mark=375-376,380,383#375 and its workaround: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/legacydb/dbmshim.c&rev=1.2&mark=325#321 So you could work around this in your "mmap io for JARs" code. This would help if there are other platforms that don't have memory-mapped IO. But I suspect OS/2 is the only platform that lacks memory-mapped IO that Firefox needs to support. Rich: thanks for your patch. The advantage of implementing a workaround in NSPR is that all consumers of NSPR will benefit.

Comment on attachment 396119 [details] [diff] [review] simulate memmap in NSPR This looks good. Thanks for the patch. >+ /* assert on PR_PROT_READWRITE which modifies the underlying file */ >+ PR_ASSERT(fmap->prot == PR_PROT_READONLY || \ >+ fmap->prot == PR_PROT_WRITECOPY); This should be backed by an if statement that return PR_FAILURE with PR_NOT_IMPLEMENTED_ERROR if fmap->prot is not one of the supported modes.

Per comment 9: > This should be backed by an if statement that return > PR_FAILURE with PR_NOT_IMPLEMENTED_ERROR if fmap->prot > is not one of the supported modes. Done. It looks a little clumsy using the same test twice but I thought it would be more meaningful than using, say, PR_ASSERT(!(fmap->prot == PR_PROT_READWRITE));

Comment on attachment 396363 [details] [diff] [review] simulate memmap - v2 Undirected review requests tend to go nowhere. I think wtc asked for this change, so ...

Rich, your patch v2 looks good. Thanks! I made some minor changes before checking it in on the NSPR trunk (NSPR 4.8.1). The only real change is to move the PR_ASSERT to the outside of the if statement. If the assertion is inside the if statement, it'll never fail. Checking in include/md/_os2.h; /cvsroot/mozilla/nsprpub/pr/include/md/_os2.h,v <-- _os2.h new revision: 3.42; previous revision: 3.41 done Checking in src/md/os2/os2misc.c; /cvsroot/mozilla/nsprpub/pr/src/md/os2/os2misc.c,v <-- os2misc.c new revision: 3.27; previous revision: 3.26 done

I will push this patch to mozilla-central this weekend.

I pushed the patch as part of the NSPR_HEAD_20090828 CVS tag to: - mozilla-central in changeset 5375876a9319 http://hg.mozilla.org/mozilla-central/rev/5375876a9319 - mozilla-1.9.2 in changeset 8227c178816b http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8227c178816b