77271 - Need to filter recursive events to prevent crashes

Status: RESOLVED WORKSFORME

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

Description

[Eric Pollmann]

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

[Brendan Eich [:brendan]]

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

Comment 2

[joki (gone)]

Moving lower priority bugs out of .9.2 milestone.

Comment 3

[Blake Ross]

Missed 0.9.3.

Comment 4

[saari (gone)]

->0.9.5

Comment 5

[joki (gone)]

Moving to 0.9.6

Comment 6

[Vladimir Ermakov]

*** Bug 102022 has been marked as a duplicate of this bug. ***

Comment 7

[Vladimir Ermakov]

*** Bug 107048 has been marked as a duplicate of this bug. ***

Comment 8

[Vladimir Ermakov]

*** Bug 76870 has been marked as a duplicate of this bug. ***

Comment 9

[joki (gone)]

*** Bug 90442 has been marked as a duplicate of this bug. ***

Comment 10

[joki (gone)]

0.9.6 has passed, moving to 0.9.7.  Load balancing 0.9.7 list shortly

Comment 11

[joki (gone)]

*** Bug 112296 has been marked as a duplicate of this bug. ***

Comment 12

[gavin long]

*** Bug 111400 has been marked as a duplicate of this bug. ***

Comment 13

[Phil Schwartau]

*** Bug 112170 has been marked as a duplicate of this bug. ***

Comment 14

[Jo Hermans]

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

[joki (gone)]

Moving bugs from 0.9.7 for triaging in 0.9.8

Comment 16

[Judson Valeski]

*** Bug 116918 has been marked as a duplicate of this bug. ***

Comment 17

[Judson Valeski]

moving to 0.9.9, though, this might have to wait until 1.0.

Comment 18

[Vladimir Ermakov]

Nominating for nsbeta1

Comment 19

[Chak Nanga]

plusing per ADT/embedding triage

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

Isn't this a dupe of bug 13350?  Or vice versa?

Comment 21

[joki (gone)]

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.

Comment 22

[Vladimir Ermakov]

*** Bug 117117 has been marked as a duplicate of this bug. ***

Comment 23

[Vladimir Ermakov]

I am not sure if this is a duplicate. I'll say it depends bug 13350.

Comment 24

[Vladimir Ermakov]

*** Bug 137458 has been marked as a duplicate of this bug. ***

Comment 25

[Garrett Smith]

"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

[Max Alekseyev]

*** Bug 176723 has been marked as a duplicate of this bug. ***

Comment 27

[Phil Schwartau]

*** Bug 187595 has been marked as a duplicate of this bug. ***

Comment 28

[Phil Schwartau]

Setting to new default owner -

Comment 29

[Simon Fraser [no longer active]]

Attachment: Testcase (warning: may crash the browser)

Comment 30

[saari (gone)]

-> bryner or maybe jkeiser?

Comment 31

[Brant Gurganus]

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.

Comment 32

[Tristan Seligmann]

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

[Max Alekseyev]

I've checked up some dupes also. The bug seems to have gone.

Comment 34

[Vladimir Ermakov]

Attachment: testcase (Mutation events)

Still crashes with recursive mutation events.

Comment 35

[Brian Ryner (not reading)]

I also just get 'Error: too much recursion' using vladimire's testcase, on a
trunk build on Linux.

Comment 36

[Brian Ryner (not reading)]

Same on Simon's testcase.  I'm going to mark this WORKSFORME.

Comment 37

[Garrett Smith]

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

[Simon Fraser [no longer active]]

Reopening based on comments. Need to test on all platforms (especially those
without bottomless stack space).

Comment 39

[timeless]

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

[Simon Fraser [no longer active]]

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

[Brendan Eich [:brendan]]

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

[Navid Zamani]

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

[Brendan Eich [:brendan]]

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

[Kevin Berkheiser]

See TB8477H

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040316

Comment 45

[Asa Dotzler [:asa]]

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]

Comment 46

[Alex Vincent [:WeirdAl]]

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

[Brendan Eich [:brendan]]

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

Comment 48

[Jesse Ruderman]

I turned both attachments into crashtests:
http://hg.mozilla.org/mozilla-central/rev/8de8dc141a7e