225423 - Loading URL crashes Moz [@ JavaObject_getPropertyById ]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Olivier Cahagne]

build ID: 20031111 on Win2k + Sun's JRE 1.4.2_02

Steps to reproduce crash:
1. Load URL http://www15.placeware.com/cc/test/check.html
2. Wait until Java loads
3. Mozilla crashes.

Doesn't crash IE6 + JRE 1.4.2_02

Comment 1

[Olivier Cahagne]

stack was generated through Dr.Watson, can someone provide a Talkback ID or
proper stack in the time being (I won't get access to such a computer today) ?

Comment 2

[Phil Schwartau]

---> Java: OJI

Comment 3

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6a) Gecko/20031030
Java Plug-in 1.4.2_02 for Netscape Navigator (DLL Helper)

I´m just trieing to get you a TalkbackID, didn´t succed so far. Loading this
page was brought my memory down, but my memory manager recovered some.
The website tells me I should use a more modern browser:

There are one or more problems preventing you from entering the meeting. Please
correct the problem(s) below, then click here to try again.

    * You must change your Platform configuration to use PlaceWare.
      You are using an unsupported browser, Netscape Communicator 5. PlaceWare
will not run using this browser. For the best experience we recommend using
Microsoft Internet Explorer 4 through 6, Netscape Communicator 4.06 through
4.79, or Netscape 7.

Comment 4

[Bill Gianopoulos [:WG9s]]

*** Bug 224781 has been marked as a duplicate of this bug. ***

Comment 5

[Bill Gianopoulos [:WG9s]]

This worked fine with the MozillaFirebird 11/03 nightly and not with the 11/04
nightly.

Comment 6

[Olivier Cahagne]

*** Bug 225696 has been marked as a duplicate of this bug. ***

Comment 7

[Olivier Cahagne]

from checkins in the regression window 11/03 to 11/04, bug 212563 could be a
culprit as the sites crashing involve creating frames through JS with
document.write.
bug 224781 comment 8 mentions the patch for bug 212563 (attachment 134397 [details] [diff] [review]) could
be wrong: anyone tried this bandaid ?

Comment 8

[Brendan Eich [:brendan]]

What's the stack backtrace?  I don't see one here.

/be

Comment 9

[Olivier Cahagne]

Brendan, see comment 1, I cannot generate a proper Talkback ID yet so I got it
from DrWatson, I will post the full stack from DrWatson but I'm not sure if it
can bring more information.

Comment 10

[Olivier Cahagne]

Attachment: DrWatson Win2k stack FB 20031113

Comment 11

[Bill Gianopoulos [:WG9s]]

Re commnet #7.

I am the person who mentioned the patch for bug 212563 as a possible cause. 
This was before this was found to be more related to java.  At the time the only
known crash was logging in to https://certadmin.entrust.com and that page:

1.  uses a java login applet
2.  the resultant page (after login) uses frames
3.  Is password protected
4.  Is SSL

Trying to find patches related to any of those, the checkin for bug 212563 kind
of stood out.

That cehcikin could still be the culprit, but based on the Dr.Watson stacktrace,
however, I am more suspicious of <a
href="http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=11%2F03%2F2003+12%3A23%3A00&maxdate=11%2F03%2F2003+12%3A24%3A00&cvsroot=%2Fcvsroot">this</a>
checkin.

Comment 12

[Brendan Eich [:brendan]]

What's the relationship between this bug and bug 222237?  Is this a DUP? 
Anyway, why is this DOM Events instead of Java: LiveConnect?

/be

Comment 13

[Bill Gianopoulos [:WG9s]]

Bug 222237 was submitted on October 15th.  This bug was not introduced until the
November 3rd/4th timeframe.  They are seperate problems.

Comment 14

[Brendan Eich [:brendan]]

Someone needs to reproduce under a debugger, and get the line number in
jsj_JavaObject.c, and ideally the bad state in registers, local memory, etc.

/be

Comment 15

[Bradley Chapman (not reading bugmail, still gone but not forever)]

I think I'm hitting this bug with all the unofficial MozillaZine nightlies
beyond 20031103. I use the P4/SSE nightlies advertised in the MozillaZine
Firebird Builds forum, and every single one beyond 20031103 hits this bug.

This is for the Linux versions of MozillaFirebird.

To reproduce:

1. Install Java SDK 1.4.2.
2. Use the Netscape6.1 gcc3.2 plugin.
3. Go to www.hushmail.com.
4. Attempt to log in with a valid HushMail account.

When the browser tries to load the HushMail.com Encryption Engine, the browser
crashes with no errors.

Comment 16

[Bill Gianopoulos [:WG9s]]

There are many sites that exhibit this issue.  However, the only one that has
been found for which you don't need a valid login to cause the crash is the one
in the original report.  The easiest way to reproduce is to just visit:

http://www15.placeware.com/cc/test/check.html

That said, the fact that you cannot reach that site is NOT why this has been
nomated as a release blocker.  There are numerous on-line banking and other
financial sites that use a JAVA Applet login that crash upon successful login. 
Also the Entrust site for certificate management (https://certadmin.entrust.com)
exhibits this problem.

Comment 17

[Bill Gianopoulos [:WG9s]]

One additional point that I left out.  This bug is NOT about
liveconnect/placeware not working with Mozilla.  That is BUG #184176.  This new
bug was introduced recently.  Before this bug was introduced, the placeware test
URL would return a page saying the browser is NOT supported.  With this bug the
browser crashes.

Comment 18

[Jon Henry]

*** Bug 226461 has been marked as a duplicate of this bug. ***

Comment 19

[Bill Gianopoulos [:WG9s]]

Backing out Brendan's 11/03/2003 12:23 PST checkin to jsinterp.c corrects this
issue.  I am not sure exactly what the issue was that this patch was supposed to
be fixing as it does not appear to be associated with any bug.

Comment 20

[Bill Gianopoulos [:WG9s]]

It appears that checkin was part of the fix for bug 196097

Comment 21

[Bill Gianopoulos [:WG9s]]

I wonder if perhaps it would be safer to put the code in jsinterp back the way
it was and add a seperate call to ComputeThis for the noSuchMethod case rather
than trying to move the existing one.  That way this change will only affect the
No Such Method case without any other side effects.

Comment 22

[Brendan Eich [:brendan]]

Taking.

/be

Comment 23

[Jerome Lacoste]

According to the changelogs, the bug brendan was working on was probably bug
196097. The fix might have been a follow up to that code.
See also bug 224956 a regression introduced by the same checkin.

Comment 24

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Will the reversion of Mr. Brendan's change affect the Linux nightly trunk?

Comment 25

[Olivier Cahagne]

*** Bug 226654 has been marked as a duplicate of this bug. ***

Comment 26

[Bernard Alleysson]

Here is the stack per Olivier request:
	jsj3250.dll!jsj_GetClassInstanceMembers()  Line 600	C++
 	jsj3250.dll!jsj_JavaInstanceMethodWrapper(JSContext * cx=0x0196c3c0, JSObject
* obj=0x01a437c0, unsigned int argc=0, long * argv=0x01ab2a2c, long *
vp=0x0012ec58)  Line 1800 + 0x11	C++
 	js3250.dll!js_Invoke(JSContext * cx=, unsigned int argc=, unsigned int flags=)
 Line 932 + 0x1c	C
 	js3250.dll!js_Interpret(JSContext * cx=0x0196c3c0, long * result=0x0012ef40) 
Line 2954	C
 	js3250.dll!js_Execute(JSContext * cx=0x00000001, JSObject * chain=0x01a443e8,
JSScript * script=0x0195b048, JSStackFrame * down=0x0012f128, unsigned int
special=32, long * result=0x0012ef40)  Line 1148	C
 	js3250.dll!obj_eval(JSContext * cx=, JSObject * obj=, unsigned int argc=, long
* argv=, long * rval=)  Line 1069 + 0x11	C
 	js3250.dll!js_Invoke(JSContext * cx=, unsigned int argc=, unsigned int flags=)
 Line 932 + 0x1c	C
 	js3250.dll!js_Interpret(JSContext * cx=0x0196c3c0, long * result=0x0012f0fc) 
Line 2954	C
 	js3250.dll!js_Invoke(JSContext * cx=, unsigned int argc=, unsigned int flags=)
 Line 949 + 0xb	C
 	js3250.dll!js_Interpret(JSContext * cx=0x0196c3c0, long * result=0x0012f304) 
Line 2954	C
 	js3250.dll!js_Invoke(JSContext * cx=, unsigned int argc=, unsigned int flags=)
 Line 949 + 0xb	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x00000000, JSObject *
obj=0x01a56820, long fval=26486216, unsigned int flags=0, unsigned int
argc=26657772, long * argv=0x0012f4cc, long * rval=0x0012f478)  Line 1026 + 0xe	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x0196c3c0, JSObject *
obj=0x01a56820, long fval=26486216, unsigned int argc=1, long * argv=0x0012f4cc,
long * rval=0x0012f478)  Line 3572 + 0x26	C
 	jsdom.dll!nsJSContext::CallEventHandler(void * aTarget=0x01a56820, void *
aHandler=0x019425c8, unsigned int argc=1, void * argv=0x0012f4cc, int *
aBoolResult=0x0012f4c8, int aReverseReturnResult=0)  Line 1245 + 0x1b	C++
 	jsdom.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x0124eb20) 
Line 182	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType(nsIDOMEventTarget *
aCurrentTarget=0x019c25c8, unsigned int aSubType=1, nsCxPusher pusher={...},
nsCOMPtr<nsIJSEventListener> jslistener={...}, nsCOMPtr<nsIScriptContext>
scriptCX={...})  Line 1420 + 0x9	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsIPresContext *
aPresContext=0x00000008, nsEvent * aEvent=0x0012f75c, nsIDOMEvent * *
aDOMEvent=0x0012f6e8, nsIDOMEventTarget * aCurrentTarget=0x019c25c8, unsigned
int aFlags=7, nsEventStatus * aEventStatus=0x0012f788)  Line 1513 + 0x24	C++
 	jsdom.dll!GlobalWindowImpl::HandleDOMEvent(nsIPresContext *
aPresContext=0x01b29840, nsEvent * aEvent=0x0012f75c, nsIDOMEvent * *
aDOMEvent=0x0012f6e8, unsigned int aFlags=1, nsEventStatus *
aEventStatus=0x0012f788)  Line 846	C++
 	gklayout.dll!DocumentViewerImpl::LoadComplete(unsigned int aStatus=0)  Line
909 + 0x48	C++
 	docshell.dll!nsDocShell::EndPageLoad(nsIWebProgress * aProgress=0x60142bbd,
nsIChannel * aChannel=0x013cbfe4, unsigned int aStatus=26514904)  Line 4326	C++
 	necko.dll!nsHttpChannel::GetURI(nsIURI * * URI=0x00000000)  Line 2615	C++
 	gklayout.dll!nsBoxFrame::GetPrefSize(nsBoxLayoutState & aBoxLayoutState={...},
nsSize & aSize={...})  Line 930 + 0x12	C++
 	gklayout.dll!nsGfxScrollFrame::GetScrollbarSizes(nsIPresContext *
aPresContext=0x77f473f3, int * aVbarWidth=0x019930e0, int *
aHbarHeight=0x00000028)  Line 324 + 0x12	C++
 	ntdll.dll!_RtlpAllocateFromHeapLookaside@4()  + 0x2d	
 	ntdll.dll!_RtlpFreeToHeapLookaside@8()  + 0x1f	
 	ntdll.dll!_RtlFreeHeap@12()  + 0xfc	
 	msvcr71.dll!free(void * pBlock=0x878b0000)  Line 103	C
 	0001fc45()	

Comment 27

[Bernard Alleysson]

Exact message is:

Unhandled exception at 0x60b56bf0 (jsj3250.dll) in MozillaFirebird.exe:
0xC0000005: Access violation reading location 0x00000015.

Code around the crash:

JavaMemberDescriptor *
jsj_GetClassInstanceMembers(JSContext *cx,
                            JNIEnv *jEnv,
                            JavaClassDescriptor *class_descriptor)
{
    if (class_descriptor->instance_members_reflected != REFLECT_COMPLETE)

60B56BF0  cmp         dword ptr [edi+14h],2 <=========== CRASH HERE

60B56BF4  push        esi  
60B56BF5  mov         esi,eax 
60B56BF7  je          jsj_GetClassInstanceMembers+52h (60B56C42h) 
        reflect_java_methods_and_fields(cx, jEnv, class_descriptor, JS_FALSE);
60B56BF9  cmp         dword ptr [edi+14h],0 
60B56BFD  jne         jsj_GetClassInstanceMembers+52h (60B56C42h) 
60B56BFF  push        0    
60B56C01  push        edi  
60B56C02  push        ebx  

Registers:

EAX = 01540938 EBX = 01A341F8 ECX = 00000000 EDX = 01ACD8C0 
ESI = 01A341F8 EDI = 00000001 EIP = 60B56BF0 ESP = 0012EBBC 
EBP = 00000001 EFL = 00010202 

00000015 = ???????? 

Comment 28

[timeless]

So class_descriptor == 0x1? cute

Comment 29

[Brendan Eich [:brendan]]

There's an ancient, ugly data dependency from ComputeThis to the
liveconnect-only clasp->convert calling code in js_Invoke, which obviously broke
when I moved the call to ComputeThis up to come early in js_Invoke.  I'm
developing a fix; I hope to have a patch by tomorrow a.m.

/be

Comment 30

[Bernard Alleysson]

*** Bug 226485 has been marked as a duplicate of this bug. ***

Comment 31

[Brendan Eich [:brendan]]

Attachment: proposed fix

Back out jsinterp.c rev 3.130, rework the necessary follow-on fix to
ComputeThis only in the JS_HAS_NO_SUCH_METHOD special case.  See the comment
there. 

Nit-picked a ComputeThis-related comment error (no code change).

/be

Comment 32

[Brendan Eich [:brendan]]

Comment on attachment 136325 [details] [diff] [review]
proposed fix

So LiveConnect lets Java methods reflect as first class JS functions, but
they're unlike JS functions in that they are "bound" to their containing object
so that object is always the |this| parameter when a method is invokved.

If you say var m = applet.method; m(), |this| will still be applet.  In JS,
|this| binds dynamically to the global object, or more precisely, the object at
the end of m's parent-linked scope chain.

Rev 1.130 moved the call to ComputeThis, the subroutine of js_Invoke that
implements most of the |this| binding logic,  up above the LiveConnect-oriented
special case that converts (via clasp->convert) a JavaMember object that stands
for a field or method (and can handle overloading) into a method-reflecting JS
function object (with the right JSFUN_BOUND_METHOD flag and parent slot).  That
left the LiveConnect code holding a bogus |this| parameter, not applet in the
example, but a global object, probably a DOM window.  Wrong-shaped private data
=> rapid death.

/be

Comment 33

[Brendan Eich [:brendan]]

I'm guilty in the last comment of misusing "dynamic" to describe JavaScript's
|this|-binding rule.  If it were dynamic in the same sense as dynamic scoping,
then you'd expect the callee's |this| to be the same as the caller's |this|, or
strictly induced by the caller's |this|.  In my opinion, it's a botch in the
Edition 3 version of the ECMA-262 spec that nested functions do not bind |this|
that way.

Instead, whether nested or not, the callee function's |this| depends on the
object to the left of the last dot in the caller's invocation expression.  If
there is no such object to the left of a dot (as in the m() case), then |this|
binds to the last object on the scope chain linked by __parent__ that starts at
the callee object.  (As usual, dot and bracket notation to access a property are
equivalent.)

JS has used this rule forever; it's necessary for top-level functions (but not
for nested functions, again in my opinion) in lieu of method declarations, in
order to let function-valued properties bind |this| to the object containing the
property, for poor-man's or class-less methods.

So |this|-binding in JS could be called "invocation-based", or some such made up
term.  Is there a better existing term in programming language theory?

/be

Comment 34

[Brendan Eich [:brendan]]

I wrote:

> If there is no such object to the left of a dot (as in the m() case), then
> |this| binds to the last object on the scope chain linked by __parent__ that
> starts at the callee object.  (As usual, dot and bracket notation to access a
> property are equivalent.)

I misspoke here.  In the m() case, the value of m, or its scope chain, does not
matter.  What matters is in which property m is bound.  If m is a local
variable, then conceptually it's the property of an activation object.  Per
ECMA-262, such objects are never exposed, even via |this| -- they are censored.
 Instead, in the spec, null is used temporarily, and when the property reference
base of null is used as a |this| parameter, the "global object" is substituted
for null.

In real-world web browsers (something neither the w3c DOM specs, nor ECMA-262,
bother to spec), there are many global objects.  Hence the use of parent-linked
scope chains to find top-level objects at the end of such chains, so that
multi-window/frame web-page JS does not suffer from dynamic scoping.  See this
comment from ComputeThis:

        /*
         * ECMA requires "the global object", but in the presence of multiple
         * top-level objects (windows, frames, or certain layers in the client
         * object model), we prefer fun's parent.  An example that causes this
         * code to run:
         *
         *   // in window w1
         *   function f() { return this }
         *   function g() { return f }
         *
         *   // in window w2
         *   var h = w1.g()
         *   alert(h() == w1)
         *
         * The alert should display "true".
         */

Anyway, whether "global" or "top-level", the point is that for an invocation
expression of the form m(), there is no object to the left of the last dot in
the callee expression (m).  So the object in which m is bound is used as |this|,
unless it's an activation object, in which case (via a null intermediate, in the
ECMA spec), the right global object will be bound to |this| in the invocation.

To reiterate the criticism of nested function |this| binding in ECMA-262 Ed. 3:
a nested function binds a property of the activation object of the outer
function, therefore, as with local var m, activation-object-censoring kicks in
and causes |this| for the inner function being invoked to bind to the global (a
global ;-) object.  Which sucks if the inner function wants |this| to bind to
the outer function's |this|, a common case.  The usual workaround is something
like this:

  function outer(a, b) {
    var self = this;
    function inner(c) {
      return self.some_method(a, b, c);
    }
    return inner;
  }

/be

Comment 35

[Mike Shaver (:shaver emeritus)]

Comment on attachment 136325 [details] [diff] [review]
proposed fix

This looks OK.	r=shaver.

Comment 36

[Asa Dotzler [:asa]]

Comment on attachment 136325 [details] [diff] [review]
proposed fix

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

Comment 37

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 38

[Hermann Schwab]

wfm Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6b) Gecko/20031125
Tinderbox BuildID 2003112523

tested URLs of dupes: Bug 224781, Bug 225696, Bug 226654 all working.

At first, testcase 1 of Bug 226485 seemed to work, but when I closed the tab,
all the contents of all other tabs was gray, contents was only redrawn in the
area used by opening and closing a menu.
Trying to reload didn´t succeed, throbber was spinning forever.
Didn´t retry.

Comment 39

[Bill Gianopoulos [:WG9s]]

re comment #38.  did you have valid logins for all the URLs you tested?  All the
dupe crashes required a login with a valid username/password to cause a crash.

Comment 40

[André Schild]

The testcases in Bug 226485 don't require a login.

Comment 41

[Hermann Schwab]

re comment 39:
no, I didn´t try to login, but two of these bugs have been crashing for me.
I don´t remember having tested Bug 224781 before.
Bug 255696 was crashing for me without logging in, before the virtual keyboard
came up, now this is working.
Bug 226654 was crashing for me, the left frame didn´t fill with content.
I didn´t report, as there were enough comments, now this is working.

I tested a bit more on Bug 226485, that one isn´t fixed for me.
Closing the browser, Java stays, closing the tab only the other tabs don´t
display. Had to reboot to get rid of Java, didn´t see it in the taskmanager.

Comment 42

[Bill Gianopoulos [:WG9s]]

Bug 224781 definitley required a login to reproduce the crash as did several
other sites (mostly web mail and banking sites that use java login applets) that
no bug was submited for.  This is actually the case that I am most interested in
making sure it is fixed, not just becuase it is my bug, but also becuase it is
not at all clear to me why a login applet would be using the live-connect feature.

Comment 43

[Bill Gianopoulos [:WG9s]]

OK my Mozilla Firebird build finally completed.  I can now verify that bug
224781 is definietly fixed by this.  I tested under original failure conditions
using Mozilla Firebird and a valid username/password.

Good job Brendan.

Comment 44

[André Schild]

Both testcases from Bug 226485 (I'm the reporter of this bug)
now work fine with 2003112608 under Windows XP

Comment 45

[WD]

*** Bug 226832 has been marked as a duplicate of this bug. ***

Comment 46

[Phil Schwartau]

Marking Verified FIXED in light of contributors' findings above.
Thanks to all -

Comment 47

[Oleg Sidletskiy]

*** Bug 226996 has been marked as a duplicate of this bug. ***

Comment 48

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Removing self from CC list.