Closed Bug 510705 Opened 10 years ago Closed 10 years ago

Simulate memory-mapped files on OS/2

Categories

(NSPR :: NSPR, enhancement)

x86
OS/2
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wuno, Assigned: dragtext)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Blocks: 504864
Version: unspecified → Trunk
 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.
Attached patch simulate memmap in NSPR (obsolete) — Splinter Review
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.
Attachment #396119 - Flags: review?(wtc)
Fix will be in NSPR.
Assignee: nobody → dragtext
Component: Networking: JAR → NSPR
Product: Core → NSPR
QA Contact: networking.jar → nspr
Version: Trunk → 4.8
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.
Attached patch simulate memmap - v2 (obsolete) — Splinter Review
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));
Attachment #396119 - Attachment is obsolete: true
Attachment #396363 - Flags: review?
Attachment #396119 - Flags: review?(wtc)
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 ...
Attachment #396363 - Flags: review? → review?(wtc)
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
Attachment #396363 - Attachment is obsolete: true
Attachment #396363 - Flags: review?(wtc)
I will push this patch to mozilla-central this weekend.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.8.1
Severity: critical → enhancement
Summary: OS/2 Minefield crash at startup → Simulate memory-mapped files on OS/2
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
You need to log in before you can comment on or make changes to this bug.