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.

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

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

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.

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

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

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.

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.

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

> 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 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 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.

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

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.

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 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 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.

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.

+ 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.

(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()

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

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 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 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)

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.

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?

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 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

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

I didn't land the "warning" patch.

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

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.

Ah, true. New bug would be good.

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.