155740 - when doing some testcase the browser crash [@js_GetProperty]

Status: VERIFIED FIXED

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

Description

[Silvia Zhao]

Steps to reproduce
1.Start netscape 7.0_02
2.Surfing the
"http://mozilla.org/quality/browser/standards/dom1/tcmatrix/index.html".
3.On the left,click "HTML AppletElement".
4.On the right,double click "hap1011.html", it will open a new window and
display is OK,then close this window.
5.Double click "hap1010.html",it will open two windows and display is OK,and
close them.
6.go to 4
7.The NS7.0_02 will be crashed ,and finding the error reason is "Segmentation
Fault".

It can be reprodeced on Mozilla 1.0.0+ Mozilla/5.0 (X11; U; SunOS sun4u; en-US;
rv:1.0.0+) Gecko/20020517 on solaris.

Comment 1

[Jay Yan]

Hi, silvia, can it be reproduced using mozilla? Bugzilla is not the place to
file Netscape bug.

Comment 2

[Olivier Cahagne]

confirming crash using build 2002070304 on Win2k (trunk) + JRE 1.4.1 beta.
Talkback ID: TB7992740W.
Perhaps should we move this to Component 'OJI'. As it's currently being
assigned, I'll leave it in 'Browser General' though.

Comment 3

[Jay Yan]

Silvia, it is OJI related bug. if so, perhaps Joshua will take care of it.

Comment 4

[Jay Yan]

sorry, Silvia, I was trying to ask whether it is a OJI-related bug in by last
comment, 
is it?

Comment 5

[Silvia Zhao]

Jay, it can be reproduced using Mozilla on Solaris. Its id in Bugtraq is
4703047.

Olivier, thanks for your comments. I am not sure whether it's related to OJI, so
marked its component as "Browser-General". I will investigate it and change its
component accordingly.

Comment 6

[Silvia Zhao]

I discussed this bug with Joshua just now. He thinks it must be related to Java,
OJI or JPI. Then I changed it to OJI.

Comment 7

[Silvia Zhao]

Jay, should I reassign the bug to Joshua or Harry? I am not familiar with Java.
Thanks.

Comment 8

[Olivier Cahagne]

also crashing on Win2k using build 2002070408 (trunk) + JRE 1.4.1: TB8015906M.

Comment 9

[Silvia Zhao]

Reassign to Joshua Xia

Comment 10

[Joshua Xia]

This is not OJI/JPI/JRE 's bug. It should be Java-script 's bug.
It crash in js/jsobj.c

Comment 11

[Joshua Xia]

It crash in jsobj.c 's "js_GetAttributes" function

Reassign to Brendan Eich

Comment 12

[Joshua Xia]

Sorry, it should be crash in jsobj.c 's "js_GetProperty" function

Comment 13

[Boris Zbarsky [:bzbarsky]]

js_GetProperty [c:/builds/seamonkey/mozilla/js/src/jsobj.c, line 2519]
js_Interpret [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 2576]
js_Execute [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 970]
JS_EvaluateUCScriptForPrincipals [c:/builds/seamonkey/mozilla/js/src/jsapi.c,
line 3381]
nsJSContext::EvaluateString
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp, line 703]
nsScriptLoader::EvaluateScript
[c:/builds/seamonkey/mozilla/content/base/src/nsScriptLoader.cpp, line 570]
nsScriptLoader::ProcessRequest
[c:/builds/seamonkey/mozilla/content/base/src/nsScriptLoader.cpp, line 478]
nsScriptLoader::ProcessScriptElement
[c:/builds/seamonkey/mozilla/content/base/src/nsScriptLoader.cpp, line 422]
nsHTMLScriptElement::MaybeProcessScript
[c:/builds/seamonkey/mozilla/content/html/content/src/nsHTMLScriptElement.cpp,
line 464]
nsHTMLScriptElement::SetDocument
[c:/builds/seamonkey/mozilla/content/html/content/src/nsHTMLScriptElement.cpp,
line 195]
nsGenericHTMLContainerElement::AppendChildTo
[c:/builds/seamonkey/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,
line 4078]
HTMLContentSink::ProcessSCRIPTTag
[c:/builds/seamonkey/mozilla/content/html/document/src/nsHTMLContentSink.cpp,
line 4995]
HTMLContentSink::AddLeaf
[c:/builds/seamonkey/mozilla/content/html/document/src/nsHTMLContentSink.cpp,
line 3280]
CNavDTD::AddLeaf [c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, line 3800]
CNavDTD::HandleScriptToken
[c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, line 2271]
CNavDTD::OpenContainer [c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp,
line 3445]
CNavDTD::HandleDefaultStartToken
[c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, line 1346]
CNavDTD::HandleStartToken
[c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp, line 1751]
CNavDTD::HandleToken [c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp,
line 912]
CNavDTD::BuildModel [c:/builds/seamonkey/mozilla/htmlparser/src/CNavDTD.cpp,
line 529]
nsParser::BuildModel [c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp,
line 1883]
nsParser::ResumeParse [c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp,
line 1747]
nsParser::ContinueParsing
[c:/builds/seamonkey/mozilla/htmlparser/src/nsParser.cpp, line 1368]
HTMLContentSink::ScriptEvaluated
[c:/builds/seamonkey/mozilla/content/html/document/src/nsHTMLContentSink.cpp,
line 4887]
nsScriptLoader::FireScriptEvaluated
[c:/builds/seamonkey/mozilla/content/base/src/nsScriptLoader.cpp, line 521]
nsScriptLoader::ProcessRequest
[c:/builds/seamonkey/mozilla/content/base/src/nsScriptLoader.cpp, line 480]
nsScriptLoader::OnStreamComplete
[c:/builds/seamonkey/mozilla/content/base/src/nsScriptLoader.cpp, line 781]
nsStreamLoader::OnStopRequest
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsStreamLoader.cpp, line 163]
nsStreamListenerTee::OnStopRequest
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsStreamListenerTee.cpp, line 66]
nsHttpChannel::OnStopRequest
[c:/builds/seamonkey/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp, line 2929]
nsOnStopRequestEvent::HandleEvent
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsRequestObserverProxy.cpp, line 213]
PL_HandleEvent [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line 597]
PL_ProcessPendingEvents [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c,
line 530]
_md_EventReceiverProc [c:/builds/seamonkey/mozilla/xpcom/threads/plevent.c, line
1078]
nsAppShellService::Run
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp, line 458]
main1 [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1472]
main [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1808]
WinMain [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1826]
WinMainCRTStartup()
KERNEL32.DLL + 0x17d08 (0x77e97d08) 

Comment 14

[timeless]

.

Comment 15

[Brendan Eich [:brendan]]

How about a little more diagnosis, please?  A crash in JS code does not mean a
bug in JS code, if calling code fed JS bad inputs.

/be

Comment 16

[Joshua Xia]

I investigated this bug, I found a very strange thing happened:

In js_GetProperty function, js_LookupProperty(cx, obj, id, &obj2, (JSProperty
**)&sprop) is invoked to get "sprop". I traced into js_LookupProperty and found
that in this function the "sprop" is set to be right value, but sometimes after
exit from js_LookupProperty, the value of "sprop" is changed to be 1. So when
execute "slot = sprop->slot;", the mozilla crash!

Comment 17

[Joshua Xia]

I think the JS Engineer should not crash even though the calling code fed JS bad
inputs.

Comment 18

[Phil Schwartau]

Reassigning to Kenton -

Comment 19

[Brendan Eich [:brendan]]

Please, enough.  This is not a JS engine bug, it's a liveconnect bug.  JS should
not and can not defend against random bogus integers cast into pointers and
returned via out parameters that should be valid JSProperty subtype pointers. 
See http://lxr.mozilla.org/mozilla/search?string=%5C%28JSProperty%5C*%5C%291 and
read bug 59686.

beard, it seems there is a case where proto_chain is non-null in
JavaObject_lookupProperty, and it points to a native JS object, in which case
returning (JSProperty*)1 in *propp is not kosher.  I'm giving you this bug, but
I'll try to suggest a fix with an untested patch, in a few minutes.

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: proposed fix (plus some whitespace cleanup)

Apply this, but for easier reviewing, read the next patch (diff -wu format).

/be

Comment 21

[Brendan Eich [:brendan]]

Attachment: diff -wu version of last patch

Straightforward fix, complicated a bit by the OBJ_DROP_PROPERTY formalism
(which is currently a stub, but I don't want to wire that knowledge into
liveconnect).

/be

Comment 22

[Brendan Eich [:brendan]]

I hope beard's around -- anyone know?

khanson, shaver: please review.

/be

Comment 23

[Phil Schwartau]

beard is still out on sabbatical; back in three or four weeks -

Comment 24

[Brendan Eich [:brendan]]

I'll take this, then, in the absence of another liveconnect backup owner.

I crave testing and code review -- phil, kenton, shaver, can you guys help?

/be

Comment 25

[Phil Schwartau]

cc'ing rogerl for possible r= on this patch, since I think 
Kenton may be starting vacation now.

I've run the patch against the entire JS and LiveConnect testsuites
on WinNT, in both the debug and optimized shells. No test regressions
were introduced by the patch - 

Comment 26

[rogerl (gone)]

Sorry, I'm not so clear on this stuff. Can you help me understand -

-what's with the '(JSProperty*)1' thing? I see it being set in LC code, but I 
don't know where/why it's being used?
-doesn't an OBJ_DROP_PROPERTY call need to be done by a caller of 
JavaObject_lookupProperty, that wouldn't have been needed before?

Comment 27

[Brendan Eich [:brendan]]

fur didn't want to make a property data structure to report a found id via
OBJ_LOOKUP_PROPERTY, so he made several of its impls in LC return 1 cast to
(JSProperty *).  That's ok so long as the object returned via *objp along with
*propp (the object in whose scope *propp exists) is non-native -- has as its
object-ops the same set of ops that includes the punning _lookupProperty impl
(e.g., JavaObject_lookupProperty).

This works because the rule for OBJ_LOOKUP_PROPERTY is that if it returns true,
then *propp will be non-null if id was found.  1 is non-null (barely :-).

The engine already takes care to OBJ_DROP_PROPERTY after
successful-and-id-was-found returns from OBJ_LOOKUP_PROPERTY calls, so there's
no need to add DROP calls outside of LC.

/be

Comment 28

[rogerl (gone)]

The part that confused me is the path followed when 'if (prop_infop->prop)' is 
true. If 'OBJ_GET_ATTRIBUTES' fails we call 'OBJ_DROP_PROPERTY', and if 
'prop_infop->wantProp' is false we call 'OBJ_DROP_PROPERTY', but otherwise it 
doesn't happen?

Comment 29

[Brendan Eich [:brendan]]

Otherwise (wantProp is true), we propagate prop back to the caller of
JavaObject_lookupProperty, which is to say, to an OBJ_LOOKUP_PROPERTY call in
the engine, which always and everywhere correctly calls OBJ_DROP_PROPERTY after
an ok return with non-null *propp.

/be

Comment 30

[rogerl (gone)]

Comment on attachment 91176 [details] [diff] [review]
diff -wu version of last patch

r=rogerl

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 91176 [details] [diff] [review]
diff -wu version of last patch

jband says sr=jband.

/be

Comment 32

[Brendan Eich [:brendan]]

browser-china Sun folks, please apply attachment 91175 [details] [diff] [review] using patch in
js/src/liveconnect and let us know that it fixes the bug.  Thanks,

/be

Comment 33

[Joshua Xia]

We have tested this patch on Solaris and Windows 2K. It works well.
Thanks

Comment 34

[chris hofmann]

Comment on attachment 91176 [details] [diff] [review]
diff -wu version of last patch

a=chofmann for 1.0.1. add the fixed1.0.1 keyword after checking into the branch

Comment 35

[Brendan Eich [:brendan]]

Fix is in the 1.0 branch.  Waiting for the 1.1 post-beta trunk to open.

/be

Comment 36

[Brendan Eich [:brendan]]

Fixed in trunk, too.

/be

Comment 37

[Phil Schwartau]

Marking Verified FIXED. 

Using current Mozilla trunk and branch builds, and following the 
steps to reproduce above, I do not crash -