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)
Core
DOM: Events
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!
Comment 1•24 years ago
|
||
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
Updated•24 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.2
Updated•24 years ago
|
Priority: -- → P2
Comment 2•23 years ago
|
||
Moving lower priority bugs out of .9.2 milestone.
Target Milestone: mozilla0.9.2 → mozilla0.9.3
Comment 6•23 years ago
|
||
*** Bug 102022 has been marked as a duplicate of this bug. ***
Comment 7•23 years ago
|
||
*** Bug 107048 has been marked as a duplicate of this bug. ***
Comment 10•23 years ago
|
||
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
Comment 11•23 years ago
|
||
*** Bug 112296 has been marked as a duplicate of this bug. ***
Comment 12•23 years ago
|
||
*** Bug 111400 has been marked as a duplicate of this bug. ***
Comment 13•23 years ago
|
||
*** Bug 112170 has been marked as a duplicate of this bug. ***
Comment 14•23 years ago
|
||
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.
Comment 15•23 years ago
|
||
Moving bugs from 0.9.7 for triaging in 0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Comment 16•23 years ago
|
||
*** Bug 116918 has been marked as a duplicate of this bug. ***
Comment 17•23 years ago
|
||
moving to 0.9.9, though, this might have to wait until 1.0.
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Comment 20•23 years ago
|
||
Isn't this a dupe of bug 13350? Or vice versa?
Comment 21•23 years ago
|
||
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
Comment 22•23 years ago
|
||
*** Bug 117117 has been marked as a duplicate of this bug. ***
Comment 23•23 years ago
|
||
I am not sure if this is a duplicate. I'll say it depends bug 13350.
Depends on: 13350
Comment 24•22 years ago
|
||
*** Bug 137458 has been marked as a duplicate of this bug. ***
Comment 25•22 years ago
|
||
"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.
Comment 26•22 years ago
|
||
*** Bug 176723 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
Target Milestone: mozilla1.0 → ---
Comment 27•22 years ago
|
||
*** Bug 187595 has been marked as a duplicate of this bug. ***
Comment 28•22 years ago
|
||
Setting to new default owner -
Assignee: joki → saari
Status: ASSIGNED → NEW
Comment 29•22 years ago
|
||
Comment 31•22 years ago
|
||
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
Comment 32•22 years ago
|
||
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?
Comment 33•22 years ago
|
||
I've checked up some dupes also. The bug seems to have gone.
Comment 34•22 years ago
|
||
Still crashes with recursive mutation events.
Assignee | ||
Comment 35•21 years ago
|
||
I also just get 'Error: too much recursion' using vladimire's testcase, on a
trunk build on Linux.
Assignee | ||
Comment 36•21 years ago
|
||
Same on Simon's testcase. I'm going to mark this WORKSFORME.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → WORKSFORME
Comment 37•21 years ago
|
||
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.
Comment 38•21 years ago
|
||
Reopening based on comments. Need to test on all platforms (especially those
without bottomless stack space).
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Comment 39•21 years ago
|
||
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.
Comment 40•21 years ago
|
||
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?
Comment 41•21 years ago
|
||
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
Comment 42•21 years ago
|
||
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 43•21 years ago
|
||
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
Comment 44•21 years ago
|
||
See TB8477H
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040316
Updated•21 years ago
|
Keywords: talkbackid
Whiteboard: TB8477H
Comment 45•21 years ago
|
||
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
Comment 46•20 years ago
|
||
Attempts to reproduce this crash in a recent Mozilla nightly and in Mozilla
Firefox 1.0 fail. I get "too much recursion" errors. :)
WFM ?
Comment 47•20 years ago
|
||
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 ago → 20 years ago
Resolution: --- → WORKSFORME
Comment 48•15 years ago
|
||
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.
Description
•