430214 - [FIX] Don't fire events while there are scriptblockers

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ideally we should make nsEventDispatcher refuse to dispatch events if there currently are scriptblockers. For now we should assert if it happens and start fixing places where it does.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marking this as [sg:critical?] I'd be surprised if there weren't exploitable bugs here.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Jonas, should this wait for bug 430215 or could I just patch nsEventDispatcher?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: like this?

So um, this does prevent events to be dispatched.
In some cases load and DOMContentLoaded, and also XUL's broadcast.
Have to fix those cases before landing, I guess.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Running chrome/browser-chrome/mochitest reveals also
selectedItemChanged, select, overflow and underflow.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we need to just assert for a while before flipping the switch. But defenitely let's try to get the assertions in and quiet asap

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch for selectedItemChanged

I don't want to remove events which have been there since 2002.
selectedItemChanged was added for XBL form controls so it isn't really used, 
but some website may use it. Trying to keep it working as well as possible.
Looking at the code reveals that selectedItemChanged is evil.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase for selectedItemChanged

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I think at least some of the load and DOMContentLoaded happen because of
XBL documents. Those documents aren't available to scripts.

Verifying and possibly modifying https://bugzilla.mozilla.org/attachment.cgi?id=348115 to handle that case.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Just debug warning

This reduces warnings by removing cases when scripts won't run, at least not
content scripts.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

> Created an attachment (id=348342) [details]
With this broadcast, ValueChange (for accessibility, I think), selectedItemChanged (attachement 348189 fixes this), select, overflow and underlow events are dispatched when running chrome/browser-chrome/mochitest.
I haven't look at 'select' yet, but otherwise should be pretty easy to fix.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fix for 'broadcast'

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 348362 [details] [diff] [review]
fix for 'broadcast'

This is a quick WIP. Should write some testcases and perhaps file a new bug for this.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 348189 [details] [diff] [review]
patch for selectedItemChanged

Ugh, we should just remove this event IMHO. I tried googling for anyone using this event, and looked at every page that google turned up that contains 'selectedItemChanged' and 'firefox' but isn't about winforms (winforms has a feature with this name). The only thing I found was the XBL forms implementation which I found mirrored on two sites.

So it seems unlikely to me that this is used.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: remove selectedItemChanged

I don't have a strong opinion about this, so I'm happy to remove.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Some tests for 'broadcast'

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 348362 [details] [diff] [review]
fix for 'broadcast'

Neil, would this be too ugly for 'broadcast'. This basically delays broadcast until the end the update and reuses nsDelayedBroadcastUpdate class.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: overflow, underflow, select with xul:tree

Note, nsContentUtils::AddScriptRunner is null-safe.

With this and other patches running chrome/browser-chrome/mochitest gives only one warning, ValueChange somewhere in browser-chrome test.

Comment 18

[Neil Deakin]

Comment on attachment 348362 [details] [diff] [review]
fix for 'broadcast'

So EndUpdate is always called after an attribute changed?

You should probably have the other Neil look at this too.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 348362 [details] [diff] [review]
fix for 'broadcast'

>So EndUpdate is always called after an attribute changed?
EndUpdate is always called whenever AttributeChange is called. Both are part of the update batch.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: warnings

With the patches there shouldn't be any warnings.
It might be better to not have the !nsContentUtils::IsChromeDoc check,
but for now it should be enough, IMO.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (nsContentUtils::IsSafeToRunScript()) {
+    CheckOverflow(parts);
+    return weakFrame.IsAlive();
+  }
+
+  nsContentUtils::AddScriptRunner(new nsOverflowChecker(this));
+  return PR_TRUE;

Why do we need the "if" block here? Can't we always just take the AddScriptRunner path?

(I do think we should return weakFrame.IsAlive() on that path though.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #21)
> Why do we need the "if" block here? Can't we always just take the
> AddScriptRunner path?
> 
> (I do think we should return weakFrame.IsAlive() on that path though.
We need to return weakFrame.IsAlive() if we always use AddScriptRunner.
But I was trying to reduce temporary objects, so that is the reason
for if()

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we care about reducing temporary objects. Let's keep the code as simple as possible.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: always with AddScriptRunner

Comment 25

[[e-mail]]

Comment on attachment 348362 [details] [diff] [review]
fix for 'broadcast'

>+    nsCOMPtr<nsIDOMElement> broadcaster = do_QueryInterface(aElement);
>+    if (broadcaster && mBroadcasterMap &&
>+        CanBroadcast(aNameSpaceID, aAttribute)) {
>         nsCOMPtr<nsIDOMElement> domele = do_QueryInterface(aElement);
Just move the existing domele up?

>                 BroadcastListener* bl =
>                     static_cast<BroadcastListener*>(entry->mListeners[i]);
> 
>                 if ((bl->mAttribute == aAttribute) ||
>                     (bl->mAttribute == nsGkAtoms::_asterix)) {
>                     nsCOMPtr<nsIContent> listener
>                         = do_QueryReferent(bl->mListener);
>+                    nsCOMPtr<nsIDOMElement> listenerEl =
>+                        do_QueryInterface(listener);
Directly query the nsIDOMElement referent?

sr=me with those fixed.

>+                ExecuteOnBroadcastHandlerFor(broadcaster,
>+                                             delayedAttrChangeBroadcasts[i].mListener,
>+                                             attrName);
I wonder why ExecuteOnBroadcastHandlerFor never used nsIContent* aListener.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fix for 'broadcast', v2

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 348565 [details] [diff] [review]
warnings

Please file a follow-up bug on getting this fixed for chrome documents.

I think the biggest reason is that if something is wrong with chrome documents, it's entirely possible that it'll be wrong for things like remote XUL and the like.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 348565 [details] [diff] [review]
warnings

(and of course hold off on checking this in until we're reasonably sure it won't fire too much)

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

The reason for checking chrome for now is the one ValueChange event which is
dispatched in DownloadProgressListener. I'm not quite sure what to do with
that.

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm sorry - been trying to read through this bug and figure it out, but it's really hard to tell. Are all these patches being submitted for approval? Are some obsolete?

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

The ones with approval1.9? are fixing possible security problems where events
are dispatched at unsafe time.

"fix for 'broadcast', V2" is the same as "fix for 'broadcast'" but with 
review comments addressed.

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348737 [details] [diff] [review]
fix for 'broadcast', v2

a191=beltzner for these three patches; should probably create a bundle so that they land together

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Actually I may want to land them separately when it is quiet time.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

I didn't land the "warning" patch.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Fixed on trunk and 1.9.1

https://bugzilla.mozilla.org/attachment.cgi?id=348428
https://bugzilla.mozilla.org/attachment.cgi?id=348637
https://bugzilla.mozilla.org/attachment.cgi?id=348737

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Wait, we still don't have code in nsEventDispatcher that refuses to dispatch events even if other code attempts to while there are script-blockers, right? (The idea being a belts-and-suspenders defense-in-depth thing since ideally no one should attempt to fire events under such circumstances)

That's what this bug originally was intended for, but I'm happy to file a new bug on that since a lot of good related work in this bug has already been done.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Ah, true. New bug would be good.

Comment 38

[[e-mail]]

Comment on attachment 348362 [details] [diff] [review]
fix for 'broadcast'

>@@ -970,56 +970,50 @@ nsXULDocument::AttributeChanged(nsIDocum
...
>             nsAutoString value;
...
>+                      nsDelayedBroadcastUpdate delayedUpdate(broadcaster,
>+                                                             listenerEl,
>+                                                             aAttribute,
>+                                                             value,
>+                                                             attrSet);
This copies value (an nsAutoString) into the mValue (an nsString), thus causing the creation of a string buffer and the copying of the value. If instead value had been made an nsString then that would have caused the creation of a string buffer, the value would have been copied there rather than an auto buffer, and then this the buffer would have been shared with mValue.