445391 - Re-enable OJI for Firefox 3.1

Status: VERIFIED FIXED

(Firefox Build System :: General, defect)

Description

[Johnny Stenback (:jst)]

Given that there's unlikely to be a NPRuntime enabled Java plugin available for the Mac in time for our release of Firefox 3.1 we need to re-enable OJI for 3.1. Once 3.1 is out the door and a Java plugin that works w/o OJI exists for the Mac too we can disable it again and finally remove it from the tree, but for now we don't really have a choice other than to re-enable OJI.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Re-enable OJI by default.

This re-enables OJI :( The bulk of this change is to the JS engine due to its header files no longer compiling cleanly on windows when included from C code. Apparently inline is not recognized as a keyword in C when using devstudio (9), so this replaces two static inline functions with macros (yay!) and moves one into a .cpp file as it wasn't used anywhere else. I'm all ears if there's a better way to do this...

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 331246 [details] [diff] [review]
Re-enable OJI by default.

Igor, could you do the review here? Thanks,

/be

Comment 3

[Igor Bukanov]

Comment on attachment 331246 [details] [diff] [review]
Re-enable OJI by default.

>diff -r eb6abd26a711 configure.in
>--- a/configure.in	Thu Jul 24 15:11:08 2008 -0700
>+++ b/configure.in	Thu Jul 24 18:07:11 2008 -0700
>@@ -4359,7 +4359,7 @@
> MOZ_NO_XPCOM_OBSOLETE=
> MOZ_NO_FAST_LOAD=
> MOZ_MEDIA=
>-MOZ_OJI=
>+MOZ_OJI=1
> MOZ_PERMISSIONS=1
> MOZ_PLACES=
> MOZ_PLAINTEXT_EDITOR_ONLY=
>@@ -5162,10 +5162,10 @@
> dnl ========================================================
> dnl = Open JVM Interface (OJI) support
> dnl ========================================================
>-MOZ_ARG_ENABLE_BOOL(oji,
>-[  --enable-oji           Enable Open JVM Integration support],
>-    MOZ_OJI=1,
>-    MOZ_OJI=)
>+MOZ_ARG_DISABLE_BOOL(oji,
>+[  --disable-oji           Disable Open JVM Integration support],
>+    MOZ_OJI=,
>+    MOZ_OJI=1)
> if test -n "$MOZ_OJI"; then
>     AC_DEFINE(OJI)
> fi
>diff -r eb6abd26a711 js/src/jsfun.cpp
>--- a/js/src/jsfun.cpp	Thu Jul 24 15:11:08 2008 -0700
>+++ b/js/src/jsfun.cpp	Thu Jul 24 18:07:11 2008 -0700
>@@ -2247,7 +2247,7 @@
> 
>     js_ReportValueError3(cx, error,
>                          (fp && fp->regs &&
>-                          StackBase(fp) <= vp && vp < fp->regs->sp)
>+                          STACK_BASE(fp) <= vp && vp < fp->regs->sp)
>                          ? vp - fp->regs->sp
>                          : (flags & JSV2F_SEARCH_STACK)
>                          ? JSDVG_SEARCH_STACK
>diff -r eb6abd26a711 js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp	Thu Jul 24 15:11:08 2008 -0700
>+++ b/js/src/jsinterp.cpp	Thu Jul 24 18:07:11 2008 -0700
>@@ -81,6 +81,18 @@
> 
> /* jsinvoke_cpp___ indicates inclusion from jsinvoke.cpp. */
> #if !JS_LONE_INTERPRET ^ defined jsinvoke_cpp___
>+
>+static inline uintN
>+GlobalVarCount(JSStackFrame *fp)

This is a wrong place for GlobalVarCount. This section on Linux and MAC does not compile in the same compilation unit as js_Interpret that uses the function. So this function should also be turned into a macro.

Comment 4

[Igor Bukanov]

(In reply to comment #1)
> Apparently inline is not recognized as a keyword in C when using devstudio (9).

Alternatively we can define something like js_static_inline that under C++ and C99 compilers will be defined as 

  #define js_static_inline static inline 

but for MSVC in C mode as

  #define js_static_inline static __inline

Note also that SpiderMonkey defines JS_INLINE, but this is to ensure that one absolutely wants to inline the function. This is not necessary with the inline functions declared in the header since in a debug build it is nice to have them as functions to be able to step through them in the debugger.

Comment 5

[Igor Bukanov]

Attachment: using __inline with MSVC in C mode

This version uses __inline when MSVC is in C mode and __inline__ for GCC to support code that compiles with -ansi with GCC. The patch also allows to redefine JS_INLINE and JS_ALWAYS_INLINE macros for performance testing.

Would it work on Windows?

Comment 6

[Igor Bukanov]

Attachment: using __inline with MSVC in C mode v1b

The new version consistently uses defined XXX , not defined(XXX), in the preprocessor conditionals.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 331292 [details] [diff] [review]
using __inline with MSVC in C  mode v1b

Works wonderfully, awesome!.

Bsmedberg, wanna glance over the configure.in changes here?

Comment 8

[Igor Bukanov]

Attachment: using __inline with MSVC in C mode v2

The previous version of the patch has a redundant static specifier in JS_ALWAYS_INLINE definition with GCC. This version fixes this.

Comment 9

[Brian Crowder]

Comment on attachment 331529 [details] [diff] [review]
using __inline with MSVC in C mode v2

As I mentioned in another bug:  I like the idea of separating ALWAYS_INLINE and INLINE, so this patch makes me very happy.

Comment 10

[Igor Bukanov]

landed - http://hg.mozilla.org/index.cgi/mozilla-central/rev/2d91c01ea27b

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This was a regression with the patch on bug 397080. Running the testcase from bug 444881 I cannot the any remaining issues with the Java Embedding Plugin 0.9.6.4 (MRJ Plugin version 1.0-JEP-0.9.6.4).

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080811020618 Minefield/3.1a2pre ID:20080811020618