Closed Bug 127243 Opened 24 years ago Closed 24 years ago

watch crashes browser on scrollTop watchpoint

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.9

People

(Reporter: giscardg, Assigned: brendan)

References

Details

(Keywords: crash, js1.5)

Attachments

(3 files, 2 obsolete files)

Reporter's HTML testcase
331 bytes, text/html
Details
corrected testcase with button above scrollbar
589 bytes, text/html
Details
don't nest scope locks in js_SetProperty; avoid re-null-testing wp->setter in js_watch_set
20.08 KB, patch
rginda
: review+
brendan
: superreview+
dbaron
: approval+
Details | Diff | Splinter Review
When trying to set a watch on the scrollTop property of an element, the browser crashes and closes. Here's a simple test case. The expected behavior is to alert the scrollTop property of the target element when the element is scrolled. <html> <script> function wee(e) { alert(e.target.scrollTop); } </script> <body onload="document.getElementById('id1').watch('scrollTop',wee)"> <div id="id1" style="width:50px;height:100px;overflow:auto"> blah<p> blah<p> blah<p> blah<p> blah<p> </div> </body> </html>
Not engine --->DOM
Assignee: rogerl → jst
Er, looks like JS Engine to mee, not DOM. Here's the stack: js_MarkGCThing(JSContext * 0x021f2648, void * 0x010dc5d0, void * 0x00000000) line 810 js_Mark(JSContext * 0x021f2648, JSObject * 0x02bb2cfc, void * 0x00000000) line 3366 + 16 bytes js_MarkGCThing(JSContext * 0x021f2648, void * 0x02ba9500, void * 0x00000000) line 841 + 21 bytes js_MarkGCThing(JSContext * 0x021f2648, void * 0x02ba9500, void * 0x00000000) line 894 + 13 bytes gc_root_marker(JSDHashTable * 0x00ff3018, JSDHashEntryHdr * 0x02b91574, unsigned long 162, void * 0x021f2648) line 950 + 14 bytes JS_DHashTableEnumerate(JSDHashTable * 0x000000a3, int (JSDHashTable *, JSDHashEntryHdr *, unsigned long, void *)* 0x010e8d2b gc_root_marker(JSDHashTable *, JSDHashEntryHdr *, unsigned long, void *), void * 0x021f2648) line 599 + 15 bytes js_GC(JSContext * 0x021f2648, unsigned int 0) line 1145 js_ForceGC(JSContext * 0x021f2648) line 974 + 12 bytes JS_GC(JSContext * 0x021f2648) line 1637 + 6 bytes nsJSContext::Notify(nsJSContext * const 0x02377f98, nsITimer * 0x023ca208) line 1573 nsTimerImpl::Process(nsTimerImpl * const 0x010dc5d0) line 286
Assignee: jst → rogerl
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: normal → critical
Keywords: crash
cc'ing Brendan, jband. Sorry, in all my bugmail, I missed this one -
Assignee: rogerl → khanson
To reproduce the crash, just load the HTML testcase above. My WinNT stack trace is the same as Johnny's: js_MarkGCThing(JSContext * 0x03614028, void * 0x00d81150, void * 0x00000000) line 813 + 3 bytes js_Mark(JSContext * 0x03614028, JSObject * 0x036a9ec0, void * 0x00000000) line 3523 + 21 bytes js_MarkGCThing(JSContext * 0x03614028, void * 0x036a9ec0, void * 0x00000000) line 844 + 37 bytes js_MarkGCThing(JSContext * 0x03614028, void * 0x036a9ee0, void * 0x00000000) line 897 + 18 bytes gc_root_marker(JSDHashTable * 0x00ca8e48, JSDHashEntryHdr * 0x03797adc, unsigned long 142, void * 0x03614028) line 953 + 18 bytes JS_DHashTableEnumerate(JSDHashTable * 0x00ca8e48, int (JSDHashTable *, JSDHashEntryHdr *, unsigned long, void *)* 0x00dc08b0 gc_root_marker(JSDHashTable *, JSDHashEntryHdr *, unsigned long, void *), void * 0x03614028) line 599 + 34 bytes js_GC(JSContext * 0x03614028, unsigned int 0) line 1153 + 21 bytes js_ForceGC(JSContext * 0x03614028) line 977 + 11 bytes JS_GC(JSContext * 0x03614028) line 1644 + 9 bytes nsJSContext::Notify(nsJSContext * const 0x02f94908, nsITimer * 0x0389b210) line 1569 + 13 bytes nsTimerImpl::Process() line 286 handleMyEvent(MyEventType * 0x038d8060) line 331 PL_HandleEvent(PLEvent * 0x038d8060) line 590 + 10 bytes PL_ProcessPendingEvents(PLEventQueue * 0x00c71430) line 520 + 9 bytes _md_EventReceiverProc(HWND__ * 0xd238010a, unsigned int 49427, unsigned int 0, long 13046832) line 1071 + 9 bytes USER32! 77e7124c() 00c71430() In the gc_root_marker frame, we were at this line in the code: GC_MARK(cx, JSVAL_TO_GCTHING(v), rhe->name ? rhe->name : "root", NULL); where we had: rhe->name 0x01004ed0 "XPCWrappedNative::mFlatJSObject" root_points_to_gcArenaPool 1
Also crashes on Linux trunk binary 20020304xx. OS: WinXP ---> All.
OS: Windows XP → All
My fault, I'm sure -- bug 62164's fix caused two other regressions. /be
Assignee: khanson → brendan
Might fix for 0.9.9, wouldn't hold things up or take a risky fix, tho. Will fix by 1.0 for sure. /be
Blocks: 122050
Status: NEW → ASSIGNED
Keywords: js1.5, mozilla1.0
Target Milestone: --- → mozilla0.9.9
I don't want to take away from your accepting blame here Brendan ;-), but this looks to be very similar to the problem we're seeing in the Siebel bug 123920.
Confirming crash with FizzillaCFM/2002020516 (0.9.8) using the Reporter's HTML Testcase. Setting Platform=All.
Hardware: PC → All
No, this is my fault, not a dup of bug 123920. Patch soon. /be
Attached patch proposed fix (obsolete) — — Splinter Review
I think watch-points were busted for properties with user-defined (callable object, not native JSPropertyOp code) setters for a while, long before bug 62164's patch landed. All should be well now. The testcase for this bug is broken in several ways: 1. The setter it uses doesn't declare the correct arguments, gets an error trying to dereference e.target, and returns no value. 2. The testcase assumes that scrolling the div's scrollbar-thumb will go through an internal equivalent of a JS assignment to the div's scrollTop property, but that's just not so. See next attachment for a working testcase, which shows the watchpoint firing when you click on the HTML button I added. /be
Scrolling the div causes no "set" of the div's scrollTop property, as expected by me and jst based on Gecko internals. If IE's scrollTop property does go through the equivalent of a JS assignment operation (how could you tell? IE lacks JS watchpoints AFAIK), file another bug. Clicking on "Press Me" alerts the new value of the scrollTop property, 42. This assignment does indeed scroll the thumb, if it's not at pixel-offset 42 relative to the div's top y coordinate. /be
Keywords: mozilla1.0 → mozilla0.9.9
I used cvs diff -20 -u to show more context, but the changes from the last patch are just these: 1. #if JS_HAS_OBJ_WATCHPOINT was being overused, it should govern only the watch and unwatch methods on Object.prototype. 2. Need to mark watchpoint-saved (and otherwise unreachable, potentially) sprops or they'll be GC'ed out from under the watchpoints! 3. Handle the JSPROP_SETTER case in js_SetProperty when recovering the original watched setter to default the new property's setter from its watched prototype's setter. Still hot for r= and sr= today for 0.9.9. /be
Attachment #73115 - Attachment is obsolete: true
Comment on attachment 73163 [details] [diff] [review] better fix, handles js_SetProperty setter inheritance and marks watchpoint-saved sprops(!) I fixed the bogus 3 in the following line to be 1 (the actual argument count for js_InternalCall that invokes the callable-object setter, if JSPROP_SETTER): + wrapper = js_NewFunction(cx, NULL, js_watch_set_wrapper, 3, 0, /be
Comment on attachment 73163 [details] [diff] [review] better fix, handles js_SetProperty setter inheritance and marks watchpoint-saved sprops(!) Hint for seeking review: explaining your patch helps a lot. =) (Delta-comments against an unexplained patch are helpful, but certainly not maximally so.) > if (!js_AddRoot(cx, &wp->closure, "wp->closure")) { > JS_free(cx, wp); > return JS_FALSE; > } > JS_APPEND_LINK(&wp->links, &rt->watchPointList); Not touched by your patch, but while I'm in here: do we want to play with roots, or just make the GC mark all the closures on the watchPointList explicitly? Especially now that we're marking the sprops anyway. (I also think that using an #ifdef test of JS_HAS_OBJ_WATCHPOINT to detect previous inclusion of jsconfig.h is asking for confusion, when we test those feature defines everywhere else as #if. Would a #define JS_CONFIG_INCLUDED in jsconfig.h kill us?)
Attachment #73163 - Flags: superreview+
Comment on attachment 73163 [details] [diff] [review] better fix, handles js_SetProperty setter inheritance and marks watchpoint-saved sprops(!) Hint for seeking review: explaining your patch helps a lot. =) (Delta-comments against an unexplained patch are helpful, but certainly not maximally so.) > if (!js_AddRoot(cx, &wp->closure, "wp->closure")) { > JS_free(cx, wp); > return JS_FALSE; > } > JS_APPEND_LINK(&wp->links, &rt->watchPointList); Not touched by your patch, but while I'm in here: do we want to play with roots, or just make the GC mark all the closures on the watchPointList explicitly? Especially now that we're marking the sprops anyway. (I also think that using an #ifdef test of JS_HAS_OBJ_WATCHPOINT to detect previous inclusion of jsconfig.h is asking for confusion, when we test those feature defines everywhere else as #if. Would a #define JS_CONFIG_INCLUDED in jsconfig.h kill us?)
Comment on attachment 73163 [details] [diff] [review] better fix, handles js_SetProperty setter inheritance and marks watchpoint-saved sprops(!) No, Mozilla, hitting enter when the focus is on a checkbox (after a failed click in the text box!) does _not_ mean to submit the form. But anyway: sr=shaver.
That #ifdef JS_HAS_OBJ_WATCHPOINT in jsdbgapi.h to hide non-public friend APIs used by other files in the engine was a fruit of my misspent youth (should have spent it in pool halls). Good point about wp->closure (whose type in memory and as a formal parameter should have been jsval, but we can't change the API now -- we can change the JSWatchPoint member type, however). Per our IRC exchange, I'll leave it to you to move the js_MarkWatchPoints call from js_SweepScopeProperties over to the mark phase of js_GC, and beef up js_MarkWatchPoints to mark closure as well as sprop. Who will r=? Time is running out for 0.9.9. /be
I'm my own worst critic. /be
Attachment #73163 - Attachment is obsolete: true
Comment on attachment 73271 [details] [diff] [review] don't nest scope locks in js_SetProperty; avoid re-null-testing wp->setter in js_watch_set shaver says his sr= still stands. /be
Attachment #73271 - Flags: superreview+
Comment on attachment 73271 [details] [diff] [review] don't nest scope locks in js_SetProperty; avoid re-null-testing wp->setter in js_watch_set r=rginda. (hey Giscard!)
Attachment #73271 - Flags: review+
Comment on attachment 73271 [details] [diff] [review] don't nest scope locks in js_SetProperty; avoid re-null-testing wp->setter in js_watch_set a=dbaron for 0.9.9 and trunk
Attachment #73271 - Flags: approval+
Fixed in 0.9.9 branch and trunk. /be
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
brendan this give the msvc warning: jsdbgapi.c X:\branch\mozilla\js\src\jsdbgapi.c(404) : warning C4550: expression evaluates to a function which is missing an argument list How about?... -extern JSPropertyOp +JSPropertyOp js_WrapWatchedSetter(JSContext *cx, jsid id, uintN attrs, JSPropertyOp setter) { JSAtom *atom; JSFunction *wrapper; if (!(attrs & JSPROP_SETTER)) - return js_watch_set; + return &js_watch_set; if (!JSVAL_IS_INT(id)) { atom = (JSAtom *)id;
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Why reopen? I thought I was writing legal C (where a function name is a pointer to it), and so does gcc. Is MSVC ass-u-ming I am so forgetful as to leave out the entire actual parameter list, including its enclosing parens? That is, does MSVC think I mean to say return js_watch_set(); or something longer? I surely didn't mean the shorest possible call, because js_watch_set is prototyped with formal parameters. Feh. Are you *sure* that return &js_watch_set; will work without warnings everywhere? Can I reassign this bug to you? I'm not going to mess with the 0.9.9 branch, and I'd like not to have branch-trunk divergence here. /be
Does parenthesizing like so: return (js_watch_set); help? /be
brendan: I don't want the bug. I was trying to be helpful. Sorry if reopening the bug bothers you. You can open a new one and assign it to the owner of the JS module if that suits you. Until now js/src has compiled without warning on VC6. Now there is a warning (and a wart'y 'extern'). FWIW, My Harbison Steele 3rd. claims the two forms are both legal and that the only difference should be if you apply the sizeof operator. (no parens make not difference to it).
Ok, I'm going to slam in this change to branch and trunk, to get rid of the warning (what's a wart-y extern?). Index: jsdbgapi.c =================================================================== RCS file: /cvsroot/mozilla/js/src/jsdbgapi.c,v retrieving revision 3.36 diff -r3.36 jsdbgapi.c 404c404 < return js_watch_set; --- > return &js_watch_set; /* & to silence schoolmarmish MSVC */ Reopening this bug does bug me and it alarmed Asa, because AFAICT you're reporting a new bug. But I'm not one to split bugs out, I have a record of morphing and diverging! /be
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
> (what's a wart-y extern?) It was in the diff in my comment: -extern JSPropertyOp +JSPropertyOp js_WrapWatchedSetter(JSContext *cx, jsid id, uintN attrs, JSPropertyOp setter) This is not the norm for this module. A quick grep shows a couple other recently added instances of this pattern. Change 'em or leave them as you will. > Reopening this bug does bug me and it alarmed Asa... Whatever. I didn't mean to alarm anyone. Seemed like a minor judgement call to me. Had this been real bustage would you rather see a new bug or just pull it in under this umbrella? I didn't see much difference between this and the case where I'd have pointed it out just before you closed the bug. It's hard to know which nits you'll care about. I know you're busy and (like me) have better things to worry about.
jband, no big deal, sorry I reacted badly -- my wrath was meant for MSVC. I think I'm going to clean out those bogo-externs for 1.0 as part of a different patch. Thanks for pointing them out. /be
Flags: testcase?
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-127243.js,v <-- regress-127243.js initial revision: 1.1
Flags: testcase? → testcase+
verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: