Closed Bug 517355 Opened 15 years ago Closed 15 years ago

Restore OJI, Liveconnect and the JEP on the 1.9.2 branch on OS X

Categories

(Core Graveyard :: Plug-ins, defect, P1)

1.9.2 Branch
All
macOS
defect

Tracking

(status1.9.2 beta1-fixed)

VERIFIED FIXED
mozilla1.9.2
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: smichaud, Assigned: smichaud)

References

Details

(Keywords: verified1.9.2)

Attachments

(3 files, 11 obsolete files)

11.43 KB, text/plain
Details
1.80 MB, application/gzip
Details
155.15 KB, patch
Details | Diff | Splinter Review
This bug has been spun off from bug 510035.

I fear we're heading for a Java train wreck on OS X on the 1.9.2
branch (what will become Firefox 3.6).  We currently don't have a
usable Java plugin for the 1.9.2 branch (or the trunk) on OS X, and
none is likely to be available before next year -- some time between
January and March, 2010, or possibly even later.  This is well after
the planned release of Firefox 3.6 (currently scheduled for November
of this year).

There are really only two feasible ways of dealing with this problem:

1) Postpone the 3.6 release until after Apple releases its port of
   Sun's Java Plugin2.

2) Restore OJI, Liveconnect and the JEP on the 1.9.2 branch.

I have no control and little influence over whether or not we choose
option 1.  But I *can* show that option 2, though somewhat clumsy, is
feasible.  In my next comment I'll post a patch that restores OJI, the
JEP and Liveconnect on the 1.9.2 branch.

A developer preview of Apple's Java Plugin2 is actually present on OS
X 10.6.X, and on OS X 10.5.8 if you've installed a recent Java update
(Update 4 or Update 5).  But it's not release quality -- Apple
acknowledges this and doesn't provide a GUI for ordinary users to turn
it on.  "Not release quality" is in fact quite an understatement --
see bug 510035 comment #41.  There's no way we can tell people to use
that Java plugin in a Firefox release.
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attached file Patch v1.0, full (obsolete) —
Here's a preliminary patch.  It's quite large, so I'm going to make it
available in three different copies -- the full patch, a full patch
without the JEP, and a patch containing only revisions to existing
files.

The full patch is the one you'd want to apply.  But the part
corresponding to the JEP is a bunch of base64-encoded files, some of
them quite large -- so this patch isn't exactly readable.

(If you don't want to bother building from the full patch, a tryserver
build should be available in a few hours.)

The full patch without the JEP contains many "new" files (those
containing the implementations of OJI and Liveconnect).  It contains
nothing but text files, so it's somewhat more readable.

But most readable of all is the copy of the patch that contains only
revisions to existing files.
Attachment #401338 - Attachment is patch: false
Attachment #401338 - Attachment mime type: text/plain → application/gzip
Attached file Patch v1.0, without JEP (obsolete) —
I should say a little about my patch:

I didn't restore things to the way they were before OJI, Liveconnect
and the JEP were removed -- there have been too many changes in the
underlying code for that to be possible (for example lots of
interfaces have been changed or removed).  Intead I made existing
objects also (conditionally) support the old interfaces.

I wrapped all my changes with define macros, so that none of them have
any effect on other platforms than OS X.

Even on the Mac, I haven't made any changes in how ordinary NPAPI
plugins are handled.

My restoration of OJI and Liveconnect are only meant to support the
JEP, and have only been tested with the JEP (on OS X 10.5.8 and
10.6.1).  If other OJI plugins existed they might cause trouble
... but I don't believe any others do exist.
Attachment #401338 - Flags: review?(jst)
Here's a tryserver build made with the full copy of my v1.0 patch:

https://build.mozilla.org/tryserver-builds/smichaud@pobox.com-restoreOJI/restoreOJI-macosx.dmg

But it somehow doesn't bundle the JEP.  So (to test it) you'll need to download a copy of JEP 0.9.7.2 from http://javaplugin.sourceforge.net and copy that to /Library/Internet Plug-Ins.

I don't know why the JEP didn't get bundled -- possibly the tryserver doesn't like base64-encoded binary files in a patch.
My tryserver build (based on my v1.0 patch) passed all tests (save for
a spurious "broken pipe" error on the Linux unit test machine).

My local build passed all tests locally, except for some reftest
failures that also happen without my patch (and so are presumably due
to problems with the tests).
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
Attached file Patch v1.1, full (obsolete) —
Here's a new patch, with some minor changes.  Once again I'm going to
post three copies of it.  A tryserver build should follow in a few
hours.

The previous patch made *almost* no changes except on the Mac, with
one very small exception -- three "extern C" JavaScript methods were
explicitly exported on all platforms.  Now these methods are only
explicitly exported on the Mac (only when OJI is defined) (this is
needed by LiveConnect).

My v1.1 patch now refuses to load any XPCOM plugin but the Java
Embedding Plugin.  If a plugin fails to load as an NPAPI plugin, the
code now checks if it's a Java plugin, an XPCOM plugin (with an
NSGetFactory() entry point), and if the plugin's name contains "Java
Embedding Plugin".  Only if all three tests pass does the plugin get
loaded.

I've also made a few other changes to tighten things up.  For example
I now always try to load a plugin as an NPAPI plugin before I try
loading it as an OJI plugin (so a plugin that supports both APIs would
have a chance to be loaded as an NPAPI plugin).  And I've made a few
more of the "old" interfaces' methods return errors if no XPCOM plugin
has been loaded or instantiated.

There's no way to stop the "old" interfaces being present -- which
might confuse plugins/extensions that test for their presence.  But,
where possible, I've made these interfaces inoperable unless they're
being used by an XPCOM plugin (i.e. by the JEP, which is the only
XPCOM plugin that can get loaded/instantiated).
Attached file Patch v1.1, without JEP (obsolete) —
Attached patch Patch v1.1, changed files only (obsolete) — Splinter Review
Attachment #401338 - Attachment is obsolete: true
Attachment #401340 - Attachment is obsolete: true
Attachment #401342 - Attachment is obsolete: true
Attachment #401338 - Flags: review?(jst)
Comment on attachment 403558 [details]
Patch v1.1, full

Josh, I'm asking you to review this patch.  But if you think parts of
it should also be reviewed by other people, please add them.
Attachment #403558 - Flags: review?(joshmoz)
Here's a tryserver build made with my full v1.1 patch:

https://build.mozilla.org/tryserver-builds/smichaud@pobox.com-bugzilla517355-v1.1/bugzilla517355-v1.1-macosx.dmg

Once again it doesn't (for some reason) bundle the JEP.  So to test it
you'll you'll need to download JEP 0.9.7.2 from
http://javaplugin.sourceforge.net and copy its binaries
(JavaEmbeddingPlugin.bundle and MRJPlugin.plugin) to /Library/Internet
Plug-Ins (or to the Contents/MacOS/plugins directory of my tryserver
build's bundle).
My v1.1 build passed all the tryserver tests except for two apparently
spurious failures on a Linux box ("Linux try hg unit test") and a
Windows box ("WINNT 5.2 try hg unit test").
Need this for beta.
Priority: -- → P1
Blocks: 519708
This patch needs at least one more revision.

PPC Macs running OS X 10.4.X (and possibly also 10.5.X) have an
ancient Apple plugin (Java Applet.plugin) that supports Java 1.3.1.
As things now stand (with my current v1.1 patch), this plugin (where
present) gets used instead of the JEP.

I've opened bug 519734 to get Java Applet.plugin blocklisted on the
1.9.2 branch.  But I need to figure out why 1.9.2-branch FF now
prefers the Java Applet.plugin over the JEP (as earlier versions of
the browser never did).

I'll be working on this today and (probably) into tomorrow.
(In reply to comment #14)
> PPC Macs running OS X 10.4.X (and possibly also 10.5.X) have an
> ancient Apple plugin (Java Applet.plugin) that supports Java 1.3.1.
> As things now stand (with my current v1.1 patch), this plugin (where
> present) gets used instead of the JEP.

Isn't this because OJI & friends aren't there, so the part of the JEP distro that handles Java 1.4 and above (even if installed manually) "can't be loaded"?  That's the way I read bug 510035 comment 4, 9, etc., and specifically bug 510035 comment 12: 

> I'd forgotten that the MRJPlugin.plugin included with the JEP is able
> to use Java 1.3.1 (where present), and when doing so doesn't use the
> OJI API.

Or have you done subsequent tests on your tryserver builds with the JEP and OJI restored and are still seeing only Java 1.3.1 loaded; comment 14 isn't clear about that.
> I'd forgotten that the MRJPlugin.plugin included with the JEP is
> able to use Java 1.3.1 (where present), and when doing so doesn't
> use the OJI API.

This comment of mine is (I now think) simply wrong.  Sorry :-(

The JEP's MRJPlugin.plugin *is* able to use Java 1.3.1 when
JavaEmbeddingPlugin.plugin isn't present.  But it still needs OJI.

Scott Field at bug 510035 was definitely running Apple's Java 1.3.1.
But I'm now pretty sure this was because he'd loaded Apple's Java
Applet.plugin -- i.e. he wasn't getting it via the JEP's
MRJPlugin.plugin.

> Or have you done subsequent tests on your tryserver builds with the
> JEP and OJI restored and are still seeing only Java 1.3.1 loaded;

Yes.

But there's more to the story.  This only happens (as I now realize)
with builds (made from my patch) that don't bundle the JEP.  In other
words, 1.9.2-branch Firefox (with my patch) only prefers Java
Applet.plugin over the JEP when both are installed to
/Library/Internet Plug-Ins/ (and no JEP exists in the distro's
Contents/MacOS/plugins directory).  This is because of a bug that's
been introduced on the 1.9.2 branch (and also the trunk) -- FF now
chooses the *older* of two plugins that support the same MIME type.

Tomorrow I'll have more to say about all this.
> FF now chooses the *older* of two plugins that support the same MIME
> type.

And have been installed to the same location (e.g. both installed to
/Library/Internet Plug-Ins/, or both installed to the distro's
Contents/MacOS/plugins/).
Attached file Patch v1.2, full (obsolete) —
Here's a new patch with just one change:

I discovered that 'make package' doesn't include the JEP in the
resulting distro.  (This is presumably the reason my previous patches'
tryserver builds didn't bundle the JEP.)

To fix this I needed to make a change to
browser/installer/package-manifest.in.

As I mentioned previously (comment #16), the problem I described in
comment #14 doesn't happen when the JEP is bundled with a 1.9.2-branch
distro (in its Contents/MacOS/plugins/ directory).  There's definitely
a bug on the trunk and 1.9.2 branch that makes the browser prefer
older plugins for a given MIME type, but it doesn't effect the JEP in
the default case (when it's bundled with FF).  So fixing this bug
doesn't depend on fixing the preference-order bug.

So I'll address the preference-order bug elsewhere -- in another bug.

I'm doing a tryserver build.  But the tryservers are severely backed
up, so it'll be quite a while before I can post a link here.
Attachment #403558 - Attachment is obsolete: true
Attachment #403561 - Attachment is obsolete: true
Attachment #403563 - Attachment is obsolete: true
Attachment #404057 - Flags: review?(joshmoz)
Attachment #403558 - Flags: review?(joshmoz)
Attached file Patch v1.2, without JEP (obsolete) —
Here's a tryserver build made with my full v1.2 patch:

https://build.mozilla.org/tryserver-builds/smichaud@pobox.com-bugzilla517355-v1.2/bugzilla517355-v1.2-macosx.dmg

This one *does* bundle the JEP.

My v1.2 patch passed all the tryserver tests except for three
apparently spurious failures -- two timeouts (on "WINNT 5.2 try hg
unit test" and "OS X 10.5.2 try hg unit test") and a "connection ended
unexpectedly" (on "Linux try hg").
Steven: Forgive my ignorance - I assume this tryserver build is supposed to show the JEP in about:plugins?

(In reply to comment #21)
> Here's a tryserver build made with my full v1.2 patch:
> 
> https://build.mozilla.org/tryserver-builds/smichaud@pobox.com-bugzilla517355-v1.2/bugzilla517355-v1.2-macosx.dmg
> 
> This one *does* bundle the JEP.
> 
> My v1.2 patch passed all the tryserver tests except for three
> apparently spurious failures -- two timeouts (on "WINNT 5.2 try hg
> unit test" and "OS X 10.5.2 try hg unit test") and a "connection ended
> unexpectedly" (on "Linux try hg").
> I assume this tryserver build is supposed to show the JEP in
> about:plugins?

It should ... but now I see that it doesn't.

You *do* see JavaEmbeddingPlugin.bundle and MRJPlugin.plugin when you
right-click on the app bundle, choose "Show Package Contents" and
browse to Contents/MacOS/plugins.  But now I see that the tryserver
didn't "build" them properly -- these two bundles are missing all
their "binaries"!!!  This must (presumably) be because the tryservers
can't deal with base64-encoded binaries in patches.

'make package' works properly with my local build (made with my v1.2
patch) -- when you install the app bundle from the resulting *.dmg
file, it contains all the needed binaries and works correctly.

For now I guess you've got to test in the following way:

1) Remove JavaEmbeddingPlugin.bundle and MRJPlugin.plugin from the
   Contents/MacOS/plugins/ directory of the tryserver build you
   downloaded and installed.

2) Download JEP 0.9.7.2 from http://javaplugin.sourceforge.net/ and
   drag its binaries (JavaEmbeddingPlugin.bundle and MRJPlugin.plugin)
   to your tryserver download's Contents/MacOS/plugins/ directory.

(I missed the lost binaries problem because I already have a copy of
the JEP in my /Library/Internet Plug-Ins/ directory, and (on my
system) the tryserver download silently failed over to using that.)
For what it's worth, here's a link to a build I made locally with 'make package':

http://people.mozilla.com/~stmichaud/bmo/firefox-517355-v1.2.dmg
It's too bad the tryservers won't let you do "push to try" on the
1.9.2 branch (only on the trunk).  That's the only way I can think of
to get around the problem of the tryservers not being able to deal
with binaries in patches.
> a bug that's been introduced on the 1.9.2 branch (and also the
> trunk) -- FF now chooses the *older* of two plugins that support the
> same MIME type.

I've opened bug 520085 to deal with this.
Attachment #404057 - Flags: review?(jst)
Comment on attachment 404057 [details]
Patch v1.2, full

I've looked over this and didn't see anything wrong but there is a lot here. The best thing to do is land it ASAP for testing but please get review from jst first.

To be clear since I'm marking this r+, this is an inherently dangerous patch. Steven did a good job writing it as far as I can tell, but this is a lot of code that nobody understood well even when it was in the tree the first time.
Attachment #404057 - Flags: review?(joshmoz) → review+
Comment on attachment 404057 [details]
Patch v1.2, full

Looks good to me. I asked Waldo to glance over the JS engine API changes here as well.
Attachment #404057 - Flags: review?(jwalden+bmo)
Attachment #404057 - Flags: review?(jst)
Attachment #404057 - Flags: review+
#endif // __cplusplus

Speaking only for JS code, I'm sure we want these to be C-style /* */ comments, as they likely cause build warnings (might not even build on particularly pedantic compilers).

Line 298-ish of the new jsfun.cpp has this:

...
}
#ifdef OJI
JS_END_EXTERN_C
#endif

#ifdef OJI
JS_BEGIN_EXTERN_C
JS_EXPORT_API(void)
#else
void
#endif
js_PutArgsObject(JSContext *cx, JSStackFrame *fp)
...

The end/begin here is pointless, right?  Please get rid of it.

The comment /* Allow inclusion from LiveConnect C files */ (passim) should include a trailing period, throughout, because it's not a sentence fragment.

With those changes js/src looks fine.
Attachment #404057 - Flags: review?(jwalden+bmo) → review+
> The end/begin here is pointless, right?

What do you mean?  JS_BEGIN_EXTERN_C and JS_END_EXTERN_C?

Are you telling me that's pointless?  I don't know myself.
I believe it should be; those macros are just gunk that expands to

extern "C" {

and

}

if __cplusplus and nothing otherwise, so I think they can be omitted.  It'd only be necessary if there were something between the two, but there's only whitespace there after the preprocessor does its thing.
Oh now I see (I think).

You're just saying I shouldn't have a JS_END_EXTERN_C just before a JS_BEGIN_EXTERN_C.  I should consolidate the two blocks.
What I'm saying is that this:

...
}
#ifdef OJI
JS_END_EXTERN_C
#endif

#ifdef OJI
JS_BEGIN_EXTERN_C
JS_EXPORT_API(void)
#else
void
#endif
js_PutArgsObject(JSContext *cx, JSStackFrame *fp)
...

expands to this in C++ with OJI:

...
}
}

extern "C" {
JS_EXPORT_API(void)
js_PutArgsObject(JSContext *cx, JSStackFrame *fp)
...

but since there's nothing between the } that closed the extern "C" started way above and the extern "C" that starts a new such block, there's no reason to close and reopen -- just do:

...
}

#ifdef OJI
JS_EXPORT_API(void)
#else
void
#endif
js_PutArgsObject(JSContext *cx, JSStackFrame *fp)
...
I understand now.  Will do.
Attached file Patch v1.3, full (obsolete) —
I've run into trouble.

There've been many changes to JS code since my v1.2 patch, and I've
had to make changes to JS header files (and sometimes to *.cpp) files
to get Liveconnect to compile and link properly.  But now I crash in a
JS trace stack every time I load a Java applet.

I don't *think* this is caused by anything in my patch (even the
newest one).  Rather I suspect there's been some kind of change to JS
tracing in the last week, and that's the source of the trouble.  But I
don't know a whole lot about JS code, so I'm asking for help from the
JS people.

Andreas Gal, your name popped up.  So I'm cc-ing you on this bug.

Thanks in advance for whatever suggestions you can come up with!

I'll post a stack trace of my crashes in a later comment.
(In reply to comment #35)
> Created an attachment (id=404993) [details]
> Patch v1.3, full
> 
> I've run into trouble.

...

>  But now I crash in a
> JS trace stack every time I load a Java applet.

Attach the stacks here.
(In reply to comment #38)
> Created an attachment (id=404995) [details]
> Gdb stack of js trace crash

That's a GC bug, confusing also called "tracer". not related to the JIT.
  2688         else
  2689             JS_TraceChildren(trc, thing, kind);
  2690     } else {

Not sure how we end up at pc == 0x00000000 with that. Maybe that gets inlined?

  2416         /* If obj has no map, it must be a newborn. */
  2417         JSObject *obj = (JSObject *) thing;
  2418         if (!obj->map)
  2419             break;
  2420         obj->map->ops->trace(trc, obj);

Maybe ops->trace is NULL? A bunch of wrappers are on the call stack. Maybe one of them isn't implemented right?
  2421         break;
I'll be using 'hg bisect' on this.  I should know more later today.
Steven, update here?
I'm closing in on the guilty patch.  I should have it within an hour.

I have no idea how close I am to a fix, though.  I'll know more once I've identified the patch that triggered the crashes.
'hg bisect' found the patch that triggers my crashes:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/042313efb6ac
"bug 511425 - removal of JSObjectOps.(get|set)RequiredSlot"
Igor Bukanov <igor@mir2.org>

I'll look through the patch for something more specific.  But I'm likely to need help from JS developers.
The first comment in bug 511425 makes me think that you should just back it out on 1.9.2 to start.
#45: thats the plan, mrbkap agrees too, working on it
Depends on: 521124
Depends on: 521135
(In reply to comment #45)
> The first comment in bug 511425 makes me think that you should just back it out
> on 1.9.2 to start.

This is not necessary, implementing JSObjectOps.trace in liveconnect is enough to fix this.
(In reply to comment #47)
> This is not necessary, implementing JSObjectOps.trace in liveconnect is enough
> to fix this.

Beat you to that one: bug 521135 has that patch.
Its not clear that that is enough though. We are crashing in the cycle collector now.
Attached file Patch v1.4, full
Here's a new revision to my patch, which updates it to current code.

It still crashes, of course -- so I won't do a tryserver build.

But here's a user repository with the patch on top of it:
http://hg.mozilla.org/users/smichaud_pobox.com/bugzilla517355/
Attachment #404057 - Attachment is obsolete: true
Attachment #404058 - Attachment is obsolete: true
Attachment #404059 - Attachment is obsolete: true
Attachment #404993 - Attachment is obsolete: true
Attachment #404994 - Attachment is obsolete: true
Depends on: 521338
I've spun off bug 521338 to follow gal's and mrbkap's current work on fixing the crashes.  mrbkap has been adding patches to my user repository (http://hg.mozilla.org/users/smichaud_pobox.com/bugzilla517355/).
After a couple hours of testing, I've concluded that the patches at
bug 521338 completely fix the problems triggered by the patch for bug
511425 (the crash I reported at comment #38, and another crash
reported at bug 521338 comment #1).

The patches at bug 521338 still need to be reviewed.  But once that
happens I think they can land on the 1.9.2 branch, either on top of or
in combination with my patch for this bug.

(My v1.4 patch will once again need to be updated to current code, but
that shouldn't be difficult -- there haven't been many changes since I
posted it.)
Landed on the 1.9.2 branch, together with mrbkap's patch for bug
521338:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3861fd3d6671
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Dbaron just pointed out to me that some OJI/Liveconnect-related leaks
were triggered by this landing:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1255123815.1255126224.11988.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1255124353.1255126125.10906.gz

I'll try to figure out what's going on next week (starting Monday).
Steven, do you have a collection of web pages which use Java Applets and we can use for testing?
Probably the best place to start is bug 371084 comment #3.  Four or
five of those applets have by now disappeared from the web, but the
rest give you quite a bit to work with.

Also look at Sun's demo applets at
http://java.sun.com/applets/jdk/1.4/index.html.  The ones I use most
often are the Clock (of course), plus ArcTest, ImageMap and Molecule
Viewer's "example3".

ImageMap is currently partially broken (in Namoroka after my patch) --
clicking on the developer's face doesn't take you to
http://java.sun.com/.  This may be a bug in the JEP, which I'll be
working on as I have the time.

Clicking on the developer's face?  Don't ask me, I didn't write it :-)
(In reply to comment #55)
> Dbaron just pointed out to me that some OJI/Liveconnect-related leaks
> were triggered by this landing:

Is there a bug filed on the leak?
Now there is -- bug 521599.
Depends on: 521675
Verified fixed on the 1.9.2 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2b1pre) Gecko/20091013 Namoroka/3.6b1pre as well as the equivalent build on 10.5 and 10.4.
Keywords: verified1.9.2
This bug is 1.9.2 only so switching to verified fixed too.
Status: RESOLVED → VERIFIED
Depends on: 523129
(Following up comment #57)

> ImageMap is currently partially broken (in Namoroka after my patch)
> -- clicking on the developer's face doesn't take you to
> http://java.sun.com/.  This may be a bug in the JEP, which I'll be
> working on as I have the time.

I've found out this is a Firefox bug, and I've opened bug 523129.
Depends on: 523625
Depends on: 521599
Depends on: 547025
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: