Closed Bug 155740 Opened 22 years ago Closed 22 years ago

when doing some testcase the browser crash [@js_GetProperty]

Categories

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

Product:
Core Graveyard
Component:
Java: Live Connect
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: silvia.zhao, Assigned: brendan)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

proposed fix (plus some whitespace cleanup)
14.06 KB, patch
Details | Diff | Splinter Review
diff -wu version of last patch
4.63 KB, patch
rogerl
: review+
brendan
: superreview+
chofmann
: approval+
Details | Diff | Splinter Review 
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.
Status: NEW → ASSIGNED
Hi, silvia, can it be reproduced using mozilla? Bugzilla is not the place to
file Netscape bug.
Severity: minor → critical
Keywords: crash
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.
Keywords: stackwanted
OS: Solaris → All
Hardware: Sun → All
Silvia, it is OJI related bug. if so, perhaps Joshua will take care of it.
sorry, Silvia, I was trying to ask whether it is a OJI-related bug in by last
comment, 
is it?
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.
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.
Component: Browser-General → OJI
Jay, should I reassign the bug to Joshua or Harry? I am not familiar with Java.
Thanks.
also crashing on Win2k using build 2002070408 (trunk) + JRE 1.4.1: TB8015906M.
Reassign to Joshua Xia
Assignee: silvia.zhao → joshua.xia
Status: ASSIGNED → NEW
This is not OJI/JPI/JRE 's bug. It should be Java-script 's bug.
It crash in js/jsobj.c
Component: OJI → JavaScript Engine
It crash in jsobj.c 's "js_GetAttributes" function

Reassign to Brendan Eich
Assignee: joshua.xia → brendan
Sorry, it should be crash in jsobj.c 's "js_GetProperty" function
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)
Keywords: stackwanted
.
Assignee: brendan → rogerl
QA Contact: asa → pschwartau
Summary: when doing some testcase the browser crash → when doing some testcase the browser crash [@js_GetProperty]
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
URL: http://mozilla.org/quality/browser/st...
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!
URL: http://mozilla.org/quality/browser/st...
I think the JS Engineer should not crash even though the calling code fed JS bad
inputs.

Reassigning to Kenton -
Assignee: rogerl → khanson
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
Assignee: khanson → beard
Component: JavaScript Engine → Live Connect
Apply this, but for easier reviewing, read the next patch (diff -wu format).

/be
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
I hope beard's around -- anyone know?

khanson, shaver: please review.

/be
Keywords: mozilla1.0.1, mozilla1.1
beard is still out on sabbatical; back in three or four weeks -
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
Assignee: beard → brendan
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 - 
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?
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
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?

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 on attachment 91176 [details] [diff] [review]
diff -wu version of last patch

r=rogerl
Attachment #91176 - Flags: review+
Comment on attachment 91176 [details] [diff] [review]
diff -wu version of last patch

jband says sr=jband.

/be
Attachment #91176 - Flags: superreview+
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
Status: NEW → ASSIGNED
We have tested this patch on Solaris and Windows 2K. It works well.
Thanks
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
Attachment #91176 - Flags: approval+
Keywords: mozilla1.0.1 → mozilla1.0.1+
Fix is in the 1.0 branch.  Waiting for the 1.1 post-beta trunk to open.

/be
Keywords: mozilla1.0.1+ → fixed1.0.1
Fixed in trunk, too.

/be
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Marking Verified FIXED. 

Using current Mozilla trunk and branch builds, and following the 
steps to reproduce above, I do not crash -
Status: RESOLVED → VERIFIED
Keywords: verified1.0.1
Product: Core → Core Graveyard
Crash Signature: [@js_GetProperty]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: