Closed Bug 77271 Opened 24 years ago Closed 20 years ago

Need to filter recursive events to prevent crashes

Categories

(Core :: DOM: Events, defect, P2)

defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: pollmann, Assigned: bryner)

References

()

Details

(Keywords: crash)

Attachments

(2 files)

This report was broken out of bug 76694. We need to implement a generic method to filter out recursive event to prevent infinite recursion and crashing. Scenario: <input type=button onclick="this.click()"> Or more complex recursion where one event handler generates an event that calls *another* event handler and this calls back to the original event. Proposed solutions: 1) Check all content methods accessible through script to verify that none of them generate events that might evaluate other event handlers. Any changes need to be verified against old browsers to ensure that old browsers also did not generate events in these cases. and/or: 2) Create a per-content-node list or hash in some generic HandleDOMEvent method of the currently-being-processed event types, and filter out any new events of these types. Comments or suggestions are appreciated!
It's pretty easy to achieve Nav 4 and earlier compatibility by looking at the lib/libmocha files returned by http://lxr.mozilla.org/classic/search?string=event_mask -- and just because you can't use a bitset for all events doesn't mean you can't for the most common ones. Use PRUint64 and the LL_* macros from "prlong.h" to cover all the cases people care about, and allocate a hash table only in the very unlikely event that the event doesn't have a bit-number < 64. /be
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.2
Priority: -- → P2
Moving lower priority bugs out of .9.2 milestone.
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Missed 0.9.3.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
->0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Moving to 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
*** Bug 102022 has been marked as a duplicate of this bug. ***
*** Bug 107048 has been marked as a duplicate of this bug. ***
Blocks: 76870
*** Bug 76870 has been marked as a duplicate of this bug. ***
*** Bug 90442 has been marked as a duplicate of this bug. ***
0.9.6 has passed, moving to 0.9.7. Load balancing 0.9.7 list shortly
Target Milestone: mozilla0.9.6 → mozilla0.9.7
*** Bug 112296 has been marked as a duplicate of this bug. ***
*** Bug 111400 has been marked as a duplicate of this bug. ***
*** Bug 112170 has been marked as a duplicate of this bug. ***
This seems to be happening quite a lot (for instance bug 112241) - if we could have a general fix for this, it could improve Zilla much.
Moving bugs from 0.9.7 for triaging in 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
*** Bug 116918 has been marked as a duplicate of this bug. ***
moving to 0.9.9, though, this might have to wait until 1.0.
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Nominating for nsbeta1
Keywords: nsbeta1
plusing per ADT/embedding triage
Keywords: nsbeta1nsbeta1+
Isn't this a dupe of bug 13350? Or vice versa?
124990 and its associated bugs is not quite ready for checkin for 0.9.9. Maintaining high priority and moving to 1.0 for completion and further testing.
Target Milestone: mozilla0.9.9 → mozilla1.0
*** Bug 117117 has been marked as a duplicate of this bug. ***
I am not sure if this is a duplicate. I'll say it depends bug 13350.
Depends on: 13350
*** Bug 137458 has been marked as a duplicate of this bug. ***
"Recursive" is only one type of loop. There are other types of loops that can cause crashes or lock up the computer while Mozilla hogs the CPU. while loops setInterval setInterval functions that cause an error may have trouble terminating. This becomes a problem because error handling is expensive. The console is obliged to print the error out each time. Also, the console may not be able to keep up with a setInterval's time, causing a stack of errors. One thing about IE is that when an error occurs, I am given an error box that allows me some options regarding this: ! A script error occured. Some scripts on the affected page may not work correctly. _________________________________________________________ [ Error details ] _________________________________________________________ Do you want to continue running scripts on the affected page? |_| Don't show errors |Source| |No| |Yes| Clicking "No" allows me to cancel out of this. Could mozilla provide a feature, say a button, perhaps, that would cancel a script? This could go right on the button bar next to stop. Cancel/CancelAll buttons could be enabled on error, and could be a preferences option.
*** Bug 176723 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.0 → ---
*** Bug 187595 has been marked as a duplicate of this bug. ***
Setting to new default owner -
Assignee: joki → saari
Status: ASSIGNED → NEW
-> bryner or maybe jkeiser?
Assignee: saari → bryner
By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and <http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss bugs are of critical or possibly higher severity. Only changing open bugs to minimize unnecessary spam. Keywords to trigger this would be crash, topcrash, topcrash+, zt4newcrash, dataloss.
Severity: major → critical
Build ID: 2003021308 OS: Windows 2000 Server This test case doesn't seem to do anything particularly nasty on my system, I just get an error in the JavaScript console saying "Too much recursion". Am I missing the point of this bug?
I've checked up some dupes also. The bug seems to have gone.
Attached file testcase (Mutation events) (deleted) —
Still crashes with recursive mutation events.
I also just get 'Error: too much recursion' using vladimire's testcase, on a trunk build on Linux.
Same on Simon's testcase. I'm going to mark this WORKSFORME.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → WORKSFORME
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4) Gecko/20030624 Both testcases failed. First testcase crashed Second testcase froze Moz and required a force quit Might want to reopen this one.
Reopening based on comments. Need to test on all platforms (especially those without bottomless stack space).
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
sfraser: jsinterp.c has a MAX_INTERP_LEVEL, for os/2 it's 250, for windows it's 30, otherwise it's 1000, perhaps 1000 isn't a good idea on macos? i had cls try changing it to 50 and he said it didn't crash. pick a number any number, experiment as much as you like. for reference, the testcase didn't crash my bezilla. note that you're welcome to and probably encouraged to file a new bug against JSEng to change that value since it isn't really relevant to the summary of this bug.
I don't have cycles to test this, but it should be pretty easy to binary search for something that doesn't crash on Mac. I'd say 300 is a reasonably limit. But I can't help wondering if there's a better way to deal with <body onload="onload()">. Can we not distinguish between the name of a JS function from a handler name? What if we munged the JS function name at load time if we detect a conflict, or just ignore it to avoid crashing?
Handler names are property names; functions are properties of the top-level object. The rest follows, it's been that way forever. Why isn't the inline_call optimization (see js_Interpret) working? If it were, I'd expect no C stack to be used, some heap used till the (inlineCallCount == MAX_INLINE_CALL_COUNT) condition is reached. /be
Please don't forget a probably more low-level call of someObj.toSource(), where someObj is something like: someObj = { child: childObj; } childObj = { daddy: someObj; } Maybe a recursion-limit would make more sense. Even cooler would be a preference where you can temporarily set the recursion level to 0 so you don't see the properties ob sub-objects. It's very painful to do a generic line of code wich alerts your current object's source out. For ex.: (not supposed to be optimal code... ;) function clickableElement(htmlElement) { this.someImportantRequiredProp = someStuffChangedFromAnotherLocation; this.child = function (evt,preparedStuff) { /* dummy */ }; // flexible handler this.handleClick = function (evt) { preparedStuff = doPreparingStuff(); evt.target.clickListener.child.handle(evt,preparedStuff); } htmlElement.clickListener = this; htmlElement.addEventListener("click",this.handleClick,true); } cd = new clickableElement(document); cd.child = { // defining a new handler daddy: cd; handle: function(evt,preparedstuff) { doSomeFancyStuffWith(daddy.someImportantRequiredProp); // alert(evt.target.clickListener.toSource()); } } Now if you want to debug something and uncomment the alert to determine if the click was called by daddy, your click will shoot firefox to death... :(
Comment 42 has nothing to do with this bug, and also seems to be confused about how Object.prototype.toSource works in SpiderMonkey. JS shell example: js> var o = {} js> var o2 = {daddy: o} js> o.child = o2 [object Object] js> o.toSource() #1={child:{daddy:#1#}} js> uneval(o) #1={child:{daddy:#1#}} js> uneval is a top-level function that calls toSource on objects, and converts non-object types to their string representations. Note the "sharp variable" syntax, after Common Lisp, used to handle cycles and join-points in the graph. There is no problem with toSource handling cycling or multiply-connected objects in SpiderMonkey. /be
See TB8477H Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040316
Keywords: talkbackid
Whiteboard: TB8477H
stacktrace from Kevin's talkback: nsXULElement::HandleDOMEvent [/mozilla/content/xul/content/src/nsXULElement.cpp, line 2675] nsXULElement::HandleDOMEvent [/mozilla/content/xul/content/src/nsXULElement.cpp, line 2831] nsXULElement::HandleDOMEvent [/mozilla/content/xul/content/src/nsXULElement.cpp, line 2831] nsXULElement::HandleDOMEvent [/mozilla/content/xul/content/src/nsXULElement.cpp, line 2831] nsXULElement::HandleDOMEvent [/mozilla/content/xul/content/src/nsXULElement.cpp, line 2831] nsXULElement::HandleDOMEvent [/mozilla/content/xul/content/src/nsXULElement.cpp, line 2831] nsXULElement::HandleChromeEvent [/mozilla/content/xul/content/src/nsXULElement.cpp, line 3995] GlobalWindowImpl::HandleDOMEvent [/mozilla/dom/src/base/nsGlobalWindow.cpp, line 886] nsDocument::HandleDOMEvent [/mozilla/content/base/src/nsDocument.cpp, line 3682] nsGenericElement::HandleDOMEvent [/mozilla/content/base/src/nsGenericElement.cpp, line 1918] nsGenericElement::HandleDOMEvent [/mozilla/content/base/src/nsGenericElement.cpp, line 1911] nsGenericDOMDataNode::HandleDOMEvent [/mozilla/content/base/src/nsGenericDOMDataNode.cpp, line 731] nsGenericElement::InsertChildAt [/mozilla/content/base/src/nsGenericElement.cpp, line 2501] nsGenericElement::doInsertBefore [/mozilla/content/base/src/nsGenericElement.cpp, line 2856] nsXMLElement::AppendChild [/mozilla/content/xml/content/src/nsXMLElement.h, line 68] XPTC_InvokeByIndex [/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102] XPCWrappedNative::CallMethod [/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2029] XPC_WN_CallMethod [/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1288] js_Invoke [/mozilla/js/src/jsinterp.c, line 943] js_Interpret [/mozilla/js/src/jsinterp.c, line 2963] js_Invoke [/mozilla/js/src/jsinterp.c, line 959] nsXPCWrappedJSClass::CallMethod [/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp, line 1338] nsXPCWrappedJS::CallMethod [/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp, line 450] PrepareAndDispatch [/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 119] SharedStub [/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 147] nsEventListenerManager::HandleEventSubType [/mozilla/content/events/src/nsEventListenerManager.cpp, line 1435] nsEventListenerManager::HandleEvent [/mozilla/content/events/src/nsEventListenerManager.cpp, line 1512] nsDocument::HandleDOMEvent [/mozilla/content/base/src/nsDocument.cpp, line 3686] nsGenericElement::HandleDOMEvent [/mozilla/content/base/src/nsGenericElement.cpp, line 1999] nsGenericElement::HandleDOMEvent [/mozilla/content/base/src/nsGenericElement.cpp, line 1989] nsGenericDOMDataNode::HandleDOMEvent [/mozilla/content/base/src/nsGenericDOMDataNode.cpp, line 759] nsGenericElement::InsertChildAt [/mozilla/content/base/src/nsGenericElement.cpp, line 2501] nsGenericElement::doInsertBefore [/mozilla/content/base/src/nsGenericElement.cpp, line 2856] nsXMLElement::AppendChild [/mozilla/content/xml/content/src/nsXMLElement.h, line 68] XPTC_InvokeByIndex [/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102] XPCWrappedNative::CallMethod [/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2029] XPC_WN_CallMethod [/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1288] js_Invoke [/mozilla/js/src/jsinterp.c, line 943] js_Interpret [/mozilla/js/src/jsinterp.c, line 2963] js_Invoke [/mozilla/js/src/jsinterp.c, line 959] nsXPCWrappedJSClass::CallMethod [/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp, line 1338] nsXPCWrappedJS::CallMethod [/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp, line 450] PrepareAndDispatch [/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 119] SharedStub [/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 147] nsEventListenerManager::HandleEventSubType [/mozilla/content/events/src/nsEventListenerManager.cpp, line 1435] nsEventListenerManager::HandleEvent [/mozilla/content/events/src/nsEventListenerManager.cpp, line 1512] nsDocument::HandleDOMEvent [/mozilla/content/base/src/nsDocument.cpp, line 3686] nsGenericElement::HandleDOMEvent [/mozilla/content/base/src/nsGenericElement.cpp, line 1999] nsGenericElement::HandleDOMEvent [/mozilla/content/base/src/nsGenericElement.cpp, line 1989] nsGenericDOMDataNode::HandleDOMEvent [/mozilla/content/base/src/nsGenericDOMDataNode.cpp, line 759] nsGenericElement::InsertChildAt [/mozilla/content/base/src/nsGenericElement.cpp, line 2501] nsGenericElement::doInsertBefore [/mozilla/content/base/src/nsGenericElement.cpp, line 2856] nsXMLElement::AppendChild [/mozilla/content/xml/content/src/nsXMLElement.h, line 68] XPTC_InvokeByIndex [/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102] XPCWrappedNative::CallMethod [/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2029] XPC_WN_CallMethod [/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1288] js_Invoke [/mozilla/js/src/jsinterp.c, line 943] js_Interpret [/mozilla/js/src/jsinterp.c, line 2963] js_Invoke [/mozilla/js/src/jsinterp.c, line 959] nsXPCWrappedJSClass::CallMethod [/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp, line 1338] nsXPCWrappedJS::CallMethod [/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp, line 450] PrepareAndDispatch [/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 119] SharedStub [/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 147] nsEventListenerManager::HandleEventSubType [/mozilla/content/events/src/nsEventListenerManager.cpp, line 1435]
Keywords: talkbackid
Whiteboard: TB8477H
Attempts to reproduce this crash in a recent Mozilla nightly and in Mozilla Firefox 1.0 fail. I get "too much recursion" errors. :) WFM ?
I think this is misassigned. It's not the bug about IE supporting two different namespaces for window objects, one for <body onload="..."> induced event handlers and the other for "function onload() {...}" handlers. It's just an old bug about JS stack limiting not being effective in all cases, on all platforms. Marking WFM per comment 46; see bug 274595 for a more specific followup bug. /be
Status: REOPENED → RESOLVED
Closed: 21 years ago20 years ago
Resolution: --- → WORKSFORME
I turned both attachments into crashtests: http://hg.mozilla.org/mozilla-central/rev/8de8dc141a7e
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: