246224 - Mozilla crashes if a chrome app uses Live Connect [@ getScriptClassLoader]

Status: RESOLVED FIXED

(Core Graveyard :: Java: Live Connect, defect)

Description

[Kevin Christmas]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040608

If XUL application, that is installed in the Mozilla/chrome directory, accesses
the java plugin through javascript, Mozilla crashes.  This does not occur if the
XUL page is not in the chrome directory.

mozilla -chrome chrome://javatest/content/javatest.xul = crash
mozilla -chrome file:///c:/javatest/javatest.xul = works

this is the text of javatest.xul
<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" >
	<box>
		<button label="click me" oncommand="var x = Packages.java.lang.String('Stupid
String'); var y = x.toString(); alert(y);"/>
	</box>
</window>

Reproducible: Always
Steps to Reproduce:
1. create a chrome application and install it in the chrome directory.  Will
crash if jar or files.
2. access java from javascript.
3.

Actual Results:  
Mozilla crashes

Expected Results:  
should have seen an alert.

currently running jdk 1.4.2_02 on windows xp.  Crashed when I tried jdk 1.5 beta
2 as well.

Comment 1

[Kevin Christmas]

Attachment: xpi file to install javatest.xul in chrome

Comment 2

[Kevin Christmas]

Attachment: simple window is javascript that accesses java

Comment 3

[Frank Wein [:mcsmurf]]

Stacktrace for this with a current cvs trunk debug build on win2k:
getScriptClassLoader(JNIEnv_ * 0x03c52798, _jobject * * 0x0012da28) line 116 +
24 bytes
ProxyFindClass(JNIEnv_ * 0x03c52798, const char * 0x03c52708) line 177 + 13 bytes
ProxyJNIEnv::FindClass(JNIEnv_ * 0x03c52798, const char * 0x03c52708) line 325 +
13 bytes
JavaPackage_resolve(JSContext * 0x038c15e8, JSObject * 0x03c3b0e0, long
0x0134ec94) line 202 + 16 bytes
_js_LookupProperty(JSContext * 0x038c15e8, JSObject * 0x03c3b0e0, long
0x02889a78, JSObject * * 0x0012dbac, JSProperty * * 0x0012db9c, const char *
0x014dcb30, unsigned int 0x00000a66) line 2518 + 42 bytes
js_GetProperty(JSContext * 0x038c15e8, JSObject * 0x03c3b0e0, long 0x02889a78,
long * 0x0012e3e0) line 2662 + 35 bytes
js_Interpret(JSContext * 0x038c15e8, long * 0x0012e580) line 3202 + 1831 bytes
js_Invoke(JSContext * 0x038c15e8, unsigned int 0x00000001, unsigned int
0x00000002) line 1301 + 13 bytes
js_InternalInvoke(JSContext * 0x038c15e8, JSObject * 0x03c3b7b8, long
0x03c3bf70, unsigned int 0x00000000, unsigned int 0x00000001, long * 0x0012e78c,
long * 0x0012e788) line 1378 + 20 bytes
JS_CallFunctionValue(JSContext * 0x038c15e8, JSObject * 0x03c3b7b8, long
0x03c3bf70, unsigned int 0x00000001, long * 0x0012e78c, long * 0x0012e788) line
3631 + 31 bytes
nsJSContext::CallEventHandler(JSObject * 0x03c3b7b8, JSObject * 0x03c3bf70,
unsigned int 0x00000001, long * 0x0012e78c, long * 0x0012e788) line 1267 + 33 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x03c75990, nsIDOMEvent
* 0x03c525e8) line 174 + 54 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x03c75a50,
nsIDOMEvent * 0x03c525e8, nsIDOMEventTarget * 0x03c52678, unsigned int
0x00000008, unsigned int 0x00000007) line 1460 + 20 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x03c75938,
nsIPresContext * 0x03823138, nsEvent * 0x0012ed98, nsIDOMEvent * * 0x0012ed40,
nsIDOMEventTarget * 0x03c52678, unsigned int 0x00000007, nsEventStatus *
0x0012ede4) line 1555
nsXULElement::HandleDOMEvent(nsIPresContext * 0x03823138, nsEvent * 0x0012ed98,
nsIDOMEvent * * 0x0012ed40, unsigned int 0x00000007, nsEventStatus * 0x0012ede4)
line 2788
PresShell::HandleDOMEventWithTarget(PresShell * const 0x03c5ddf8, nsIContent *
0x03c75798, nsEvent * 0x0012ed98, nsEventStatus * 0x0012ede4) line 6109
nsButtonBoxFrame::MouseClicked(nsIPresContext * 0x03823138, nsGUIEvent *
0x0012efe4) line 178
nsButtonBoxFrame::HandleEvent(nsButtonBoxFrame * const 0x03cb1888,
nsIPresContext * 0x03823138, nsGUIEvent * 0x0012efe4, nsEventStatus *
0x0012f4d8) line 150
PresShell::HandleEventInternal(nsEvent * 0x0012efe4, nsIView * 0x00000000,
unsigned int 0x00000001, nsEventStatus * 0x0012f4d8) line 6073 + 39 bytes
PresShell::HandleEventWithTarget(PresShell * const 0x03c5ddf8, nsEvent *
0x0012efe4, nsIFrame * 0x03cb1888, nsIContent * 0x03c75798, unsigned int
0x00000001, nsEventStatus * 0x0012f4d8) line 5984 + 22 bytes
nsEventStateManager::CheckForAndDispatchClick(nsIPresContext * 0x03823138,
nsMouseEvent * 0x0012f700, nsEventStatus * 0x0012f4d8) line 2962 + 66 bytes
nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x03823a98,
nsIPresContext * 0x03823138, nsEvent * 0x0012f700, nsIFrame * 0x03cb1888,
nsEventStatus * 0x0012f4d8, nsIView * 0x036ae740) line 1969 + 23 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012f700, nsIView * 0x036ae740,
unsigned int 0x00000001, nsEventStatus * 0x0012f4d8) line 6081 + 52 bytes
PresShell::HandleEvent(PresShell * const 0x03c5de6c, nsIView * 0x036ae740,
nsGUIEvent * 0x0012f700, nsEventStatus * 0x0012f4d8, int 0x00000001, int &
0x00000001) line 5922 + 25 bytes
nsViewManager::HandleEvent(nsView * 0x036ae740, nsGUIEvent * 0x0012f700, int
0x00000001) line 2212
nsViewManager::DispatchEvent(nsViewManager * const 0x036ae590, nsGUIEvent *
0x0012f700, nsEventStatus * 0x0012f5d8) line 1952 + 20 bytes
HandleEvent(nsGUIEvent * 0x0012f700) line 79
nsWindow::DispatchEvent(nsWindow * const 0x03c533ac, nsGUIEvent * 0x0012f700,
nsEventStatus & nsEventStatus_eIgnore) line 1060 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f700) line 1081
nsWindow::DispatchMouseEvent(unsigned int 0x0000012d, unsigned int 0x00000000,
nsPoint * 0x00000000) line 5204 + 21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 0x0000012d, unsigned int
0x00000000, nsPoint * 0x00000000) line 5457
nsWindow::ProcessMessage(unsigned int 0x00000202, unsigned int 0x00000000, long
0x00140021, long * 0x0012fc28) line 3953 + 28 bytes
nsWindow::WindowProc(HWND__ * 0x0027029c, unsigned int 0x00000202, unsigned int
0x00000000, long 0x00140021) line 1342 + 27 bytes
USER32! 77e01ef0()
USER32! 77e0204c()
USER32! 77e021af()
nsAppShellService::Run(nsAppShellService * const 0x01379e88) line 524
main1(int 0x00000001, char * * 0x00263f50, nsISupports * 0x01341008) line 1334 +
32 bytes
main(int 0x00000001, char * * 0x00263f50) line 1811 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e81af6()

Comment 4

[timeless]

skid marks:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/modules/oji/src/ProxyClassLoader.cpp&mark=59,68,70,85,111,116#59

cause:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/caps/src/nsPrincipal.cpp&rev=1.32&mark=459,461,463#450

caller and callee disagree about return convention.

Comment 5

[timeless]

Attachment: null check GetURI out param...

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 150662 [details] [diff] [review]
null check GetURI out param...


What about merging

+		 if (NS_FAILED(rv)) return rv;
+		 if (!*result) return NS_ERROR_NOT_AVAILABLE;

into

+		 if (NS_SUCCEEDED(rv) && !*result) return
NS_ERROR_NOT_AVAILABLE;

?

Comment 7

[timeless]

serge: what about not doing that? you lose information about the failure by
doing it.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> serge: what about not doing that? you lose information about the failure by
> doing it.

Which information does the merging loose ?
(I'm sorry that I don't figure it out :-()

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #8)
> Which information does the merging loose ?
> (I'm sorry that I don't figure it out :-()

the rv that the function returned

Comment 10

[Frank Wein [:mcsmurf]]

Alright this patch makes Mozilla not crash and it also displays "Stupid String"
in the alert (the only difference between loaded from file:// and chrome:// is
that from file it displays as alert title [JavaScript Application] and from
chrome only Alert, but this is probably intended and not part of this bug).

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> (In reply to comment #8)
> > Which information does the merging loose ?
> > (I'm sorry that I don't figure it out :-()
> 
> the rv that the function returned

Oh ? I must be missing something !

The merged patch would be
{{
-                return principal->GetURI(result);
+                nsresult rv = principal->GetURI(result);
+		 if (NS_SUCCEEDED(rv) && !*result) return NS_ERROR_NOT_AVAILABLE;
+                return rv;
}}

Why would the |rv| value be changed by the |if| call ?

Comment 12

[timeless]

sorry, i'm a literalist, what you wrote was:

> What about merging
> +		 if (NS_FAILED(rv)) return rv;
> +		 if (!*result) return NS_ERROR_NOT_AVAILABLE;
> into
> +		 if (NS_SUCCEEDED(rv) && !*result) return
NS_ERROR_NOT_AVAILABLE;

you made no mention of the return that followed. you were not merging the lines
you specified. yes your change would be fine. please don't attach a new patch we
still need caillon to sign off on the design.

further there's no guarantee that someone won't decide to put code between the
if (...) return and the final return (although it's not very likely). while it
would be their responsibility to notice that the return is being used for both
success and failure, it isn't guaranteed that they would, and the result would
probably be as bad as the crash we have today. some optimizations just aren't
worth it, let the compiler/optimizer figure them out.

Comment 13

[Jack Lu]

I will work with Kyle to resolve this issue ASAP.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 150662 [details] [diff] [review]
null check GetURI out param...

I don't mind if you plant some assertions in getScriptCodebase() in addition to
your existing patch (which I don't mind that much).  But make sure you are not
running with the system principal before calling getScriptCodebase().

Comment 15

[Asa Dotzler [:asa]]

We've got a very small window (a couple of hours) to get a patch and reviews
here if this is going to make Mozilla 1.7.

Comment 16

[Kyle Yuan]

actully, the entire ProxyFindClass function is totally unuseful. I'm going to
remove it. new patch is coming.

Comment 17

[Kyle Yuan]

Attachment: get rid if ProxyClassLoader

The ProxyFindClass relys on a java class netscape.oji.ProxyClassLoader to
lookup a class. But that class	is totally unsupported in modern jre. So we
don't need that function any more.

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 150662 [details] [diff] [review]
null check GetURI out param...


Based on an email from Christian,
this patch would become
{{
-		 return principal->GetURI(result);
+		 nsresult rv = principal->GetURI(result);
+		 if (NS_SUCCEEDED(rv) && !*result)
+		   return NS_ERROR_UNEXPECTED;
+		 return rv;
}}
(style and value error changes)

Kyle:
I'm posting it as a comment, since you are working on an new patch.
Please, see how it applies to what you are preparing.

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #18)
> (From update of attachment 150662 [details] [diff] [review])
> 
> Kyle:
> I'm posting it as a comment, since you are working on an new patch.
> Please, see how it applies to what you are preparing.

The answer is simple: irrelevant :->

***

Then, in your patch, you may want to remove the following variable too:
{{ <http://lxr.mozilla.org/mozilla/search?string=mInProxyFindClass>
/modules/oji/src/ProxyJNI.cpp, line 284 -- jbool mInProxyFindClass;
/modules/oji/src/ProxyJNI.cpp, line 1707 -- : mSecureEnv(secureEnv),
mContext(NULL), mInProxyFindClass(JNI_FALSE)
}}

Comment 20

[Jack Lu]

Even though 'ProxyFindClass' may not supported in the current JRE, I think it 
still would be better to leave it as it is. It was added to support loading Java 
Package from cutomer website (See 77194).

Could anyone here point to me why 'FindClass' fails to find java.lang.String? I 
think we should nail down the root cause instead of add defensive check ( I am 
not saying it is not a right thing to do) or removing some unused code.

Comment 21

[Kyle Yuan]

Xiaobin, the crash happened before FindClass tried to find java.lang.String.
JavaPackage_resolve gets called for every "dot", i.e. java. java.lang,
java.lang.String.

Serge, you are right. If we removed ProxyClassLoader, we should also remove
mInProxyFindClass.

Comment 22

[Kyle Yuan]

(In reply to comment #20)
> Even though 'ProxyFindClass' may not supported in the current JRE, I think it 
> still would be better to leave it as it is. It was added to support loading Java 
> Package from cutomer website (See 77194).
> 

I can't imagine how it can help on this now. see
http://lxr.mozilla.org/seamonkey/source/modules/oji/src/ProxyClassLoader.cpp#125,
 do you think user will be able to successfully load
netscape.oji.ProxyClassLoader someday?

Comment 23

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> sorry, i'm a literalist, what you wrote was:
> 
> > What about merging
> > +		 if (NS_FAILED(rv)) return rv;
> > +		 if (!*result) return NS_ERROR_NOT_AVAILABLE;
> > into
> > +		 if (NS_SUCCEEDED(rv) && !*result) return
> NS_ERROR_NOT_AVAILABLE;
> 
> you made no mention of the return that followed.

Because I was not touching it.
(I guess that this is where all the confusion started :-<)

> you were not merging the lines you specified.

Were I not ?

> yes your change would be fine. please don't attach a new patch we
> still need caillon to sign off on the design.

"Done" :->

> further there's no guarantee that someone won't decide to put code between the
> if (...) return and the final return (although it's not very likely). while it
> would be their responsibility to notice that the return is being used for both
> success and failure, it isn't guaranteed that they would, and the result would
> probably be as bad as the crash we have today. some optimizations just aren't
> worth it, let the compiler/optimizer figure them out.

I understand; yet the optimizer does not help me to read the code.
Further, wouldn't a final |return NS_OK;| have been equivalent and much more
explicit ?
(But less likely to be optimized !? Oh well ;-()

Anyway...

Comment 24

[Kyle Yuan]

Attachment: patch for 1.7 branch

Discussed with Xiaobin, we agree that to keep the change minimal, it's better
to comment out the (temporarily) unused code instead of remove them. 
This patch is for 1.7 branch only. Will do a better job on the trunk.

Comment 25

[Xiaobin Lu]

Comment on attachment 150756 [details] [diff] [review]
patch for 1.7 branch

It will be better if we can add :

if (NS_FAILED(result))
  outClass = NULL;

return outClass;

I will approve this if make the above change or similar.

Comment 26

[Kyle Yuan]

Comment on attachment 150756 [details] [diff] [review]
patch for 1.7 branch

I'll do what Xiaobin said when I check it in.

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 150756 [details] [diff] [review]
patch for 1.7 branch

sr=jst

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 150756 [details] [diff] [review]
patch for 1.7 branch

a=asa (on behalf of drivers) for checkin to 1.7

Comment 29

[Kyle Yuan]

fixedin 1.7 branch. will make a new patch for trunk.

Comment 30

[Jacek Piskozub]

It seems all blocking1.7+ bugs (including this one) are actually fixed in 1.7
branch. Do we really need the blocking1.7 flags, then?

Comment 31

[Serge Gautherie (:sgautherie)]

(In reply to comment #30)
> It seems all blocking1.7+ bugs (including this one) are actually fixed in 1.7
> branch. Do we really need the blocking1.7 flags, then?

'fixed1.7' keyword means that 'blocking1.7+' has been dealt with:
the current situation is as it should be.

(Look at the related requests on the "Release-Status" page; which I don't have
the link at hand.)

Comment 32

[Jacek Piskozub]

Its http://www.mozilla.org/roadmap/release-status.html and I read it before
commenting. 

The document does not state explicitely whether blocking1.7+ flags should stay
or be removed after fixed1.7+ is set. However, the actual practice is to remove
them. At this moment there are only 5 left from 20+. So why leave the final five
misleading people about the progress towards the release?

Sorry for the spam.
 

Comment 33

[Serge Gautherie (:sgautherie)]

(In reply to comment #32)
> However, the actual practice is to remove them.

Ah ? I was not aware of this.
(I guessed there was a sort of global 'blocking' flag (value and item) removal
after the release. My mistake.)

> At this moment there are only 5 left from 20+. So why leave the final five
> misleading people about the progress towards the release?

May be these should be checked/set 'verified1.7' before removing 'blocking1.7+' ??

Anyway, some of the drivers are in the CC list:
I'll let them comment on this issue from now.

Comment 34

[David Epstein]

Verified on Mozilla 1.7 branch. Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.7) Gecko/20040617. command-line and link for test case return the string.

Comment 35

[Kyle Yuan]

Attachment: patch for trunk

Comment 36

[Xiaobin Lu]

Comment on attachment 151409 [details] [diff] [review]
patch for trunk

ProxyJNI.cpp change looks good to me. I am not expert in JS security field. You
may need to find appropriate person to review the other change.

Comment 37

[Xiaobin Lu]

Comment on attachment 151409 [details] [diff] [review]
patch for trunk

ProxyJNI.cpp change looks good to me. I am not expert in JS security field. You
may need to find appropriate person to review the other change.

Comment 38

[Xiaobin Lu]

Comment on attachment 151409 [details] [diff] [review]
patch for trunk

ProxyJNI.cpp change looks good to me. I am not expert in JS security field. You
may need to find appropriate person to review the other change.

Comment 39

[Kyle Yuan]

Comment on attachment 151409 [details] [diff] [review]
patch for trunk

Okay, Chris, could you please review my change of getScriptCodebase?

Comment 40

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 151409 [details] [diff] [review]
patch for trunk

I think that getScriptCodebase's caller should probably explicitly handle what
should happen if we are running with the system principal.  Perhaps that means
a better fix is removing getScriptCodebase entirely, since it only has the one
caller and moving the functionality in getScriptCodebase directly into the
caller.  What do you think, Kyle?

Comment 41

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 151409 [details] [diff] [review]
patch for trunk

Unsetting flag pending response.

Comment 42

[Kyle Yuan]

Attachment: patch for trunk (v2)

removed the getScriptCodebase function, moved it into getScriptClassLoader, and
adjusted the code order - find netscape.oji.ProxyClassLoaderFactory first then
get the codebase - that can save much time because finding
netscape.oji.ProxyClassLoaderFactory will be always failed.

Comment 43

[Kyle Yuan]

Attachment: patch for trunk (v2)

sorry, the previous patch is wrong.

Comment 44

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 152138 [details] [diff] [review]
patch for trunk (v2)

>+    PRBool equals;
>+    rv = principal->Equals(sysprincipal, &equals);
>+    // Can't get URI from system principal
>+    if (NS_SUCCEEDED(rv) && equals)
>+        return NS_ERROR_FAILURE;

You return errors for failure everywhere except here.  if
nsIPrincipal::Equals() fails here, you will continue on.  That seems like you
want to do:

    if (NS_FAILED(rv)) return rv;
    if (equals) return NS_ERROR_FAILURE;


Is this something that support is desired for in the future?  If so, an XXX
comment may be in order.

r=caillon.

Comment 45

[Kyle Yuan]

Comment on attachment 152138 [details] [diff] [review]
patch for trunk (v2)

I'll address caillon's comment when checkin.

> Is this something that support is desired for in the future?  If so, an XXX
comment may be in order.

As far as I know, the answer is no. netscape.oji.ProxyClassLoaderFactory will
likely not be supported in any subsequent (Sun) JREs.

Comment 46

[Johnny Stenback (:jst)]

Comment on attachment 152138 [details] [diff] [review]
patch for trunk (v2)

sr=jst

Comment 47

[timeless]

Comment on attachment 152138 [details] [diff] [review]
patch for trunk (v2)

mozilla/modules/oji/src/ProxyClassLoader.cpp	1.16

Comment 48

[Kyle Yuan]

Comment on attachment 152138 [details] [diff] [review]
patch for trunk (v2)

I think you should mark the bug as FIXED rather than making this patch
obsolete.

Comment 49

[Kyle Yuan]

fix checked in by timeless.