Closed Bug 77271 Opened 19 years ago Closed 15 years ago

Need to filter recursive events to prevent crashes

Categories

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

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.
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: 17 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: 17 years ago15 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.