Last Comment Bug 390488 - [FIXr]Event listeners should have access to the JS callstack
: [FIXr]Event listeners should have access to the JS callstack
Status: RESOLVED FIXED
: fixed1.8.0.13, fixed1.8.1.8, regression, testcase
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: mozilla1.9alpha8
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
data:text/html;charset=utf-8,%3C!DOCT...
Depends on:
Blocks: 371858
  Show dependency treegraph
 
Reported: 2007-08-01 10:55 PDT by John Mellor (Jomel)
Modified: 2008-01-09 06:36 PST (History)
6 users (show)
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase Extension (1.54 KB, application/x-xpinstall)
2007-08-01 10:55 PDT, John Mellor (Jomel)
no flags Details
Proposed fix (6.64 KB, patch)
2007-08-01 15:04 PDT, Boris Zbarsky [:bz]
jst: review+
brendan: superreview+
dveditz: approval1.8.1.8+
dveditz: approval1.8.0.13+
jst: approval1.9+
Details | Diff | Review

Description John Mellor (Jomel) 2007-08-01 10:55:59 PDT
Created attachment 274807 [details]
Testcase Extension

As of Firefox 2.0.0.5 rc1 [1], chrome event listeners can no longer obtain the stacktrace that led to the event, whether they use (new Error).stack or arguments.callee.caller[.caller...].

These were useful for dump debugging and even in extension code to adjust behaviour based on the source of events.

The change was since Firefox 2.0.0.4 rc3 [2], possibly due to bug 326777 [3].
According to Boris Zbarsky on IRC, this effect was unintended however, and event listeners should continue to be able to obtain stack traces.

I've attached a tiny testcase extension, just install then click Tools->Test Event Stacktraces (an extension was the easiest way to test with chrome privileges).

[1]: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2.0.0.5-candidates/rc1/unsigned/firefox-2.0.0.5.en-US.win32.zip
[2]: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2.0.0.4-candidates/rc3/unsigned/firefox-2.0.0.4.en-US.win32.zip
[3]: Relevant checkins are: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=bzbarsky%25mit.edu&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-10+14%3A17&maxdate=2007-07-10+14%3A24&cvsroot=%2Fcvsroot
Comment 1 Boris Zbarsky [:bz] 2007-08-01 11:13:18 PDT
So this is a regression from bug 371858 of sorts: when event handling pushes its JSContext on the stack, we save off its stackframe, even though this JSContext is already what's at the top of the stack.

It's easy enough to detect this case and not save off the stack, and I think for branch we should do exactly that...
Comment 2 John Mellor (Jomel) 2007-08-01 11:23:18 PDT
I've added a simpler content only testcase to the URL field. This simulates a
click, and checks that it can detect that it did so.
Comment 3 Boris Zbarsky [:bz] 2007-08-01 15:04:18 PDT
Created attachment 274833 [details] [diff] [review]
Proposed fix

The idea here is to not clear out the JS frame stack if the context being
pushed is what's already at that top.  We'd still clear it when pushing a null
context, of course.
Comment 4 Boris Zbarsky [:bz] 2007-08-01 22:07:16 PDT
Comment on attachment 274833 [details] [diff] [review]
Proposed fix

Fix for a regression from one of our security fixes.  Risk of a security regression is small, and there is basically no risk of functionality regressions.
Comment 5 Boris Zbarsky [:bz] 2007-08-06 19:05:35 PDT
Checked in, with the tests.
Comment 6 Daniel Veditz [:dveditz] 2007-08-07 20:05:40 PDT
Comment on attachment 274833 [details] [diff] [review]
Proposed fix

approved for 1.8.0.13 and 1.8.1.7, a=dveditz
Comment 7 Boris Zbarsky [:bz] 2007-08-07 20:53:19 PDT
Fixed on both branches

Note You need to log in before you can comment on or make changes to this bug.