Last Comment Bug 531176 - Add assertion that it's safe to run scripts before dispatching any events (and fix the cases when events are dispatched at unsafe time)
: Add assertion that it's safe to run scripts before dispatching any events (an...
Status: RESOLVED FIXED
[sg:critical?]
: fixed1.9.0.20, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- minor (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
: Andrew Overholt [:overholt]
Mentors:
: 496366 (view as bug list)
Depends on: 535887 535926 553808 557398
Blocks: 496366 515190
  Show dependency treegraph
 
Reported: 2009-11-25 15:21 PST by Justin Lebar (not reading bugmail)
Modified: 2010-07-23 12:42 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.4+
.4-fixed
.10+
.10-fixed


Attachments
patch (6.61 KB, patch)
2009-11-26 09:09 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
patch (6.58 KB, patch)
2009-11-26 09:41 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
+script error (20.66 KB, patch)
2009-11-27 06:54 PST, Olli Pettay [:smaug]
jonas: review+
jst: superreview+
Details | Diff | Splinter Review
for 1.9.2 (20.46 KB, patch)
2010-03-16 09:28 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
for 1.9.2 without the assertion (19.46 KB, patch)
2010-03-16 13:52 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
for 1.9.2 without the assertion (19.46 KB, patch)
2010-03-16 14:17 PDT, Olli Pettay [:smaug]
mbeltzner: approval1.9.2.4+
Details | Diff | Splinter Review
for 1.9.1 without the assertion and without the FocusBlurEvent (17.39 KB, patch)
2010-03-16 14:25 PDT, Olli Pettay [:smaug]
jst: review+
mbeltzner: approval1.9.1.10+
dveditz: approval1.9.0.next+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2009-11-25 15:21:15 PST
It doesn't appear that we check nsContentUtils::IsSafeToRunScripts() before running the event dispatch code.  We probably should add an assertion to this effect to help catch unsafe event dispatches.
Comment 1 Olli Pettay [:smaug] 2009-11-26 09:09:11 PST
Created attachment 414740 [details] [diff] [review]
patch

Posted this to tryserver.
Passes at least mochitest locally.
Comment 2 Olli Pettay [:smaug] 2009-11-26 09:10:22 PST
The problematic cases I found weren't common while running mochitest.
Comment 3 Olli Pettay [:smaug] 2009-11-26 09:14:16 PST
Comment on attachment 414740 [details] [diff] [review]
patch

> #ifdef DEBUG
>+    nsresult rv = NS_ERROR_FAILURE;
>+    if (target->GetContextForEventHandlers(&rv) ||
>+        NS_FAILED(rv)) {
>+      NS_ERROR("This is unsafe!");
>+    }

It is ok to dispatch events to non-scriptable, data-only documents, which
don't have context for script event handlers. This is basically XBL documents, where
load event is used to detect that they are loaded.
Comment 4 Olli Pettay [:smaug] 2009-11-26 09:41:58 PST
Created attachment 414746 [details] [diff] [review]
patch

This should compile even on scratchbox
Comment 5 Olli Pettay [:smaug] 2009-11-27 06:54:10 PST
Created attachment 414850 [details] [diff] [review]
+script error

I found yet another event; error event from scripts.
This contains some tests for error event.
Comment 6 Daniel Veditz [:dveditz] 2009-11-30 12:47:45 PST
Does the bug summary need to be updated? Seems like you've gone beyond adding a helpful assert to fixing security vulns uncovered by the assert.
Comment 7 Jesse Ruderman 2009-12-06 12:23:07 PST
> +      NS_ERROR("This is unsafe!");

Please use a more descriptive assertion message, such as "dispatching an event when it is not safe to run script".
Comment 8 Jesse Ruderman 2009-12-06 12:29:27 PST
Please consider using separate, public bugs for the changes other than adding the assertion.  If they cause regressions, having a public bug will make tracking easier, draw less attention to the security aspect, and make life less painful if we have to back something out.
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-12-08 23:00:46 PST
Comment on attachment 414850 [details] [diff] [review]
+script error

So this wouldn't fire assertions if the event is dispatched against chrome documents?

I wonder if we ideally would like to assert event then, just to avoid having to teach developers which events are unsafe to do what from.

Do you know which events fire at chrome while there are script blockers?

r=me to land this for now anyhow.
Comment 10 Olli Pettay [:smaug] 2009-12-08 23:04:17 PST
(In reply to comment #9)
> (From update of attachment 414850 [details] [diff] [review])
> So this wouldn't fire assertions if the event is dispatched against chrome
> documents?

Yes it does fire the assertion. It doesn't fire assertion with
non-scriptable documents. Basically data documents which don't have
script handling object.
Comment 11 Justin Lebar (not reading bugmail) 2009-12-08 23:06:32 PST
Um...did adding a blocking item just un-secure this bug?  I really didn't mean to do that.
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-12-08 23:16:35 PST
(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 414850 [details] [diff] [review] [details])
> > So this wouldn't fire assertions if the event is dispatched against chrome
> > documents?
> 
> Yes it does fire the assertion. It doesn't fire assertion with
> non-scriptable documents. Basically data documents which don't have
> script handling object.

Why do you need that distinction? Is this related to loading of XBL-documents by any chance?
Comment 13 Olli Pettay [:smaug] 2009-12-08 23:17:48 PST
(In reply to comment #12)
> Why do you need that distinction? Is this related to loading of XBL-documents
> by any chance?
Yes. We load XBL documents which don't have script handling object but still
use (C++) event listener to get the load event.
Comment 14 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-12-08 23:18:37 PST
(In reply to comment #11)
> Um...did adding a blocking item just un-secure this bug?  I really didn't mean
> to do that.

Anyone that doesn't have access to this bug won't be able to see any sensitive information about the bug. Just the bug number and if it's resolved or not IIRC.
Comment 15 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-12-08 23:21:05 PST
That's what I recall from adding this assertion synchronously...

It's annoying to add an exception just for this one case. Though I guess its ok since we won't ever run any scripted event handlers, is that correct? Also, ideally the sync loading of XBL should go away eventually.
Comment 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-12-09 10:09:27 PST
Another thing that would be good to do is to add assertions is to the mutation events code. Currently we'll only trigger the assertion added in your patch if there actually are mutation event listeners since we won't attempt to fire the event otherwise.

I think this can be accomplished by adding an assertion to nsContentUtils::HasMutationListeners that checks if all current blockers are removable.

IIRC I found one case in the cssframeconstructor that triggered such an assertion, which was easily fixable by setting aNotify to false or some such.

Do you want a separate bug for this?
Comment 17 Olli Pettay [:smaug] 2009-12-09 10:15:21 PST
I'll do that in a separate bug.
Comment 18 Olli Pettay [:smaug] 2009-12-11 09:07:56 PST
jst, want to sr? This is a security sensitive bug, so needs r and sr.
Comment 19 Johnny Stenback (:jst, jst@mozilla.com) 2009-12-17 12:41:37 PST
Comment on attachment 414850 [details] [diff] [review]
+script error

Looks good. sr=jst
Comment 20 Olli Pettay [:smaug] 2009-12-18 11:35:58 PST
http://hg.mozilla.org/mozilla-central/rev/e58c2ef1d65b
Comment 21 David Baron :dbaron: ⌚️UTC-10 2010-03-15 16:26:20 PDT
Have you considered taking this on branches?  It appears to fix bug 496366.
Comment 22 Olli Pettay [:smaug] 2010-03-15 16:31:11 PDT
Yeah, we should probably take this to branches. Not sure about the assertion, 
but the other parts of the patch. I'll update the patch for branches.
(3.5 will need significant changes)
Comment 23 Olli Pettay [:smaug] 2010-03-16 09:28:07 PDT
Created attachment 432835 [details] [diff] [review]
for 1.9.2

I'll upload this to tryserver
Comment 24 Olli Pettay [:smaug] 2010-03-16 13:52:29 PDT
Created attachment 432907 [details] [diff] [review]
for 1.9.2 without the assertion

I think we may not want to add the assertion to branches.
Comment 25 Olli Pettay [:smaug] 2010-03-16 14:17:25 PDT
Created attachment 432914 [details] [diff] [review]
for 1.9.2 without the assertion

a merge problem fixed; ValueChange should bubble.
Comment 26 Olli Pettay [:smaug] 2010-03-16 14:25:27 PDT
Created attachment 432918 [details] [diff] [review]
for 1.9.1 without the assertion and  without the FocusBlurEvent

I'm posting this to tryserver.

Since 1.9.1 doesn't have the focusmanager, the FocusBlurEvent part doesn't really
apply to that branch.

Otherwise only minor changes to trunk patch.

Fixes Bug 496366.
Comment 27 Daniel Veditz [:dveditz] 2010-03-19 14:03:20 PDT
Comment on attachment 432914 [details] [diff] [review]
for 1.9.2 without the assertion

Are bug 535887 and bug 535926 from the "depends on" field regressions that need to be fixed before taking this on branches?
Comment 28 Olli Pettay [:smaug] 2010-03-19 14:11:12 PDT
Those aren't regressions. They are cases where the assertion fires, but the
assertion itself doesn't change behavior.

The patch did change other behavior, but I haven't heard of regressions
related to those changes.
Comment 29 Olli Pettay [:smaug] 2010-03-21 12:21:58 PDT
*** Bug 496366 has been marked as a duplicate of this bug. ***
Comment 30 Mike Beltzner [:beltzner, not reading bugmail] 2010-03-24 12:54:54 PDT
Comment on attachment 432914 [details] [diff] [review]
for 1.9.2 without the assertion

a=beltzner for 1.9.2.3 and 1.9.1.10
Comment 32 Al Billings [:abillings] 2010-04-07 16:45:29 PDT
Is there a way to easily test this fix?

It looks like bug 496366 can be used, per comment 21. Are we sure it is this checkin that fixed that bug?
Comment 33 Olli Pettay [:smaug] 2010-04-08 02:58:58 PDT
(In reply to comment #32)
> Is there a way to easily test this fix?
> 
> It looks like bug 496366 can be used, per comment 21.
Yeah, there is a testcase in that bug.

> Are we sure it is this
> checkin that fixed that bug?
Yes.
Comment 34 Al Billings [:abillings] 2010-04-08 15:41:56 PDT
Verified for 1.9.2 via the testcase in bug 496366 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.4pre) Gecko/20100408 Lorentz/3.6.4pre (.NET CLR 3.5.30729). 

Verified for 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.10pre) Gecko/20100407 Shiretoko/3.5.10pre (.NET CLR 3.5.30729).
Comment 35 Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-07-18 22:02:34 PDT
Comment on attachment 432918 [details] [diff] [review]
for 1.9.1 without the assertion and  without the FocusBlurEvent

Requesting approval1.9.0.next on this patch so that we can take it in upcoming Camino 2.0.x security and stability updates.  If approved, I'll handle the checkins, unless the patch author requests otherwise.
Comment 36 Daniel Veditz [:dveditz] 2010-07-22 19:21:02 PDT
Comment on attachment 432918 [details] [diff] [review]
for 1.9.1 without the assertion and  without the FocusBlurEvent

Approved for 1.9.0.20, a=dveditz
Comment 37 Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-07-23 12:42:51 PDT
Checking in dom/src/base/nsGlobalWindow.cpp;
/cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v  <--  nsGlobalWindow.cpp
new revision: 1.1027; previous revision: 1.1026
done
Checking in dom/src/base/nsJSEnvironment.cpp;
/cvsroot/mozilla/dom/src/base/nsJSEnvironment.cpp,v  <--  nsJSEnvironment.cpp
new revision: 1.401; previous revision: 1.400
done
Checking in dom/tests/mochitest/bugs/Makefile.in;
/cvsroot/mozilla/dom/tests/mochitest/bugs/Makefile.in,v  <--  Makefile.in
new revision: 1.32; previous revision: 1.31
done
RCS file: /cvsroot/mozilla/dom/tests/mochitest/bugs/test_bug531176.html,v
done
Checking in dom/tests/mochitest/bugs/test_bug531176.html;
/cvsroot/mozilla/dom/tests/mochitest/bugs/test_bug531176.html,v  <--  test_bug531176.html
initial revision: 1.1
done
Checking in layout/forms/nsComboboxControlFrame.cpp;
/cvsroot/mozilla/layout/forms/nsComboboxControlFrame.cpp,v  <--  nsComboboxControlFrame.cpp
new revision: 1.432; previous revision: 1.431
done

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