when doing some testcase the browser crash [@js_GetProperty]

VERIFIED FIXED

Status

--
critical
VERIFIED FIXED
16 years ago
8 years ago

People

(Reporter: silvia.zhao, Assigned: brendan)

Tracking

({crash})

Trunk
crash

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Updated

16 years ago
Status: NEW → ASSIGNED

Comment 1

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

Updated

16 years ago
Severity: minor → critical
Keywords: crash

Comment 2

16 years ago
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

Comment 3

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

Comment 4

16 years ago
sorry, Silvia, I was trying to ask whether it is a OJI-related bug in by last
comment, 
is it?
(Reporter)

Comment 5

16 years ago
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.
(Reporter)

Comment 6

16 years ago
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
(Reporter)

Comment 7

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

Comment 8

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

Comment 9

16 years ago
Reassign to Joshua Xia
Assignee: silvia.zhao → joshua.xia
Status: ASSIGNED → NEW

Comment 10

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

Updated

16 years ago
Component: OJI → JavaScript Engine

Comment 11

16 years ago
It crash in jsobj.c 's "js_GetAttributes" function

Reassign to Brendan Eich
Assignee: joshua.xia → brendan

Comment 12

16 years ago
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

Comment 14

16 years ago
.
Assignee: brendan → rogerl
QA Contact: asa → pschwartau
Summary: when doing some testcase the browser crash → when doing some testcase the browser crash [@js_GetProperty]
(Assignee)

Comment 15

16 years ago
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

16 years ago
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

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

Comment 18

16 years ago
Reassigning to Kenton -
Assignee: rogerl → khanson
(Assignee)

Comment 19

16 years ago
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
(Assignee)

Updated

16 years ago
Component: JavaScript Engine → Live Connect
(Assignee)

Comment 20

16 years ago
Created attachment 91175 [details] [diff] [review]
proposed fix (plus some whitespace cleanup)

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

/be
(Assignee)

Comment 21

16 years ago
Created attachment 91176 [details] [diff] [review]
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
(Assignee)

Comment 22

16 years ago
I hope beard's around -- anyone know?

khanson, shaver: please review.

/be
Keywords: mozilla1.0.1, mozilla1.1

Comment 23

16 years ago
beard is still out on sabbatical; back in three or four weeks -
(Assignee)

Comment 24

16 years ago
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

Comment 25

16 years ago
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

16 years ago
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?
(Assignee)

Comment 27

16 years ago
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

16 years ago
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?
(Assignee)

Comment 29

16 years ago
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

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

r=rogerl
Attachment #91176 - Flags: review+
(Assignee)

Comment 31

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

jband says sr=jband.

/be
Attachment #91176 - Flags: superreview+
(Assignee)

Comment 32

16 years ago
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

Comment 33

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

Comment 34

16 years ago
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+

Updated

16 years ago
Keywords: mozilla1.0.1 → mozilla1.0.1+
(Assignee)

Comment 35

16 years ago
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
(Assignee)

Comment 36

16 years ago
Fixed in trunk, too.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 37

16 years ago
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

Updated

8 years ago
Component: Java: Live Connect → Java: Live Connect
Product: Core → Core Graveyard
Crash Signature: [@js_GetProperty]
You need to log in before you can comment on or make changes to this bug.