Jesse Ruderman Reporter
	Description • 17 years ago

Loading the testcase triggers:

###!!! ASSERTION: This is unsafe: 'nsContentUtils::IsSafeToRunScript()', file /Users/jruderman/trunk/mozilla/layout/base/nsDocumentViewer.cpp, line 1051

###!!! ASSERTION: How did we get here if it's not safe to run scripts?: 'nsContentUtils::IsSafeToRunScript()', file /Users/jruderman/trunk/mozilla/layout/base/nsPresShell.cpp, line 5567

These assertions were added in bug 401155.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 17 years ago

Need to test this after bug 395609 and bug 420415

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 16 years ago

I don't get any assertions anymore with the testcase.
Jesse, do you still see those?

	Jesse Ruderman Reporter
	Comment 3 • 16 years ago

WFM on trunk.

	Jesse Ruderman Reporter
	Comment 4 • 16 years ago

Now I see this again, and the dialog that appears is blank.  (Change from bug 423355?)

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 5 • 16 years ago

This is actually a completely new bug rather than the problem that originally existed. But lets reuse the bug since it's already been reopened.

Yeah, this is a regression from bug 423355. What happens is that in
nsGenericElement::doReplaceOrInsertBefore we do

mozAutoDocUpdate updateBatch(aDocument, UPDATE_CONTENT_MODEL,
    aReplace || nodeType == nsIDOMNode::DOCUMENT_FRAGMENT_NODE);

However this is not a replacement nor an insertion of a document fragment, so the last argument is false. This means that we don't call BeginUpdate, but we do call nsContentUtils::AddScriptBlocker.

When we then a little later in the function insert the new node that starts and ends a BeginUpdate/EndUpdate pair. When that EndUpdate is called it is the last update and so we start doing a bunch of XBL stuff and a bunch of frameloader stuff. This is "safe" since the last update has actually ended, but there updateBatch above is still scriptblocking and so this assertion fires :(

I guess the best fix for now is to simply change that updatebatch to always pass aNotify=true. Ideally more code should be moved out of using the last EndUpdate to instead use scriptrunners. But that won't happen for 1.9

	Jesse Ruderman Reporter
	Comment 6 • 16 years ago

I think this is supposed to pop up a dialog asking whether I want to launch Adium or cancel.  Instead, it just asserts:

###!!! ASSERTION: This is unsafe: 'nsContentUtils::IsSafeToRunScript()', file /Users/jruderman/trunk/mozilla/layout/base/nsDocumentViewer.cpp, line 1054

(but doesn't trigger the other assertion)

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 7 • 16 years ago

Weird, I would not have though that we would block any dialogs. It's possible that that is unrelated.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 16 years ago

The other option is to skip the update batch here completely in the cases when we'll only have one update...

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 9 • 16 years ago

Not really sure how to do that without adding another argument to mozAutoDocUpdate though.

But really, does it really add much overhead to do an extra BeginUpdate/EndUpdate. I would imagine most subscribers to those notifications only to heavy lifting on the last EndUpdate and/or first BeginUpdate anyway.


I'm starting to get nervous about that we still, this close to release still have cases where we run script with scriptblockers on the stack. As things currently stand script running while there are scriptblockers will be a bit horked. We actively check for scriptblockers in three cases where we don't really need to:

* We won't ever flush so such script will get erroneous values for .offsetLeft etc.
* We refuse to fire mutation events for any mutations such script cases.
* PresShells won't handle any events. I think this might mean that if the script does an alert() the user won't be able to click the OK button in the alert.

Do we want to revert these checks so that *if* we have some cases where script execute at the wrong time we'll still work mostly ok. We're not really winning anything security wise since if script is already running, running more script isn't going to allow any new vectors for attacks.

The one thing I guess I could see keeping is the check for mutation events since they are much more rarely used, and keeping that check will actually prevent certain bugs from creating exploitable attack vectors (i.e. layout causing mutation events during reflow or frame construction).


Or we can just keep things the way they are now (but still fix this bug of course) and hope that we catch any remaining script running while there are script blockers before shipping. Jesse has tended to find these pretty fast through his normal testing since we tend to assert pretty quickly when things go wrong.

Input very welcome.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 16 years ago

Could just skip the auto-helper in this case and do things by hand, I suppose.  But you might be right that the cost of an extra wrapper update batch is not that big: likely just a dozen or two virtual function calls...

> We're not really winning anything security wise since if script is already
> running, running more script isn't going to allow any new vectors for attacks.

This is true, but there are non-script things that can trigger layout flushes, for example.  So just letting through layout flushes isn't quite right.

It might make some sense to pop off all pending script blockers if we actually go to execute script, since in that case they clearly aren't helping anymore.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 11 • 16 years ago

(In reply to comment #10)
> Could just skip the auto-helper in this case and do things by hand, I suppose. 

That would be hard with all the early-returns in there. Which is why we have the helpers in the first place :)

> But you might be right that the cost of an extra wrapper update batch is not
> that big: likely just a dozen or two virtual function calls...

Hmm.. maybe a non-trivial amount I suppose. I wonder if optimizers would appropriately optimize away if I just added another argument with a default value. I suppose they would, constant propagation is a pretty trivial optimization these days...

> > We're not really winning anything security wise since if script is already
> > running, running more script isn't going to allow any new vectors for attacks.
> 
> This is true, but there are non-script things that can trigger layout flushes,
> for example.  So just letting through layout flushes isn't quite right.

But those things were issues before too, so we're not off any worse than we were a few weeks ago.

Generally when we do these sorts of things we add the assertion first and make sure we get to a point where we are fairly sure the assertion won't trigger. *then* we make the switch to actually act on the assertion.

> It might make some sense to pop off all pending script blockers if we actually
> go to execute script, since in that case they clearly aren't helping anymore.

Do we have a good place to do this? Especially given that we knowingly don't block chrome script (such as contentpolicies) with scriptblockers, just content script. Also I am more than a little worried about mucking around with this too much at this point.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 16 years ago

> But those things were issues before too, so we're not off any worse than we
> were a few weeks ago.

Yeah, probably true.

> Also I am more than a little worried about mucking around with this too
> much at this point.

Me too...

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 13 • 16 years ago

So this creates a new mozAutoDocConditionalContentUpdateBatch class that only calls BeginUpdate/EndUpdate if aNotify is true. It never manually adds scripblockers otherwise.

I also removed the places where we refuse to do things when there are scriptblockers there. I think it's the right thing to do. I left the assertions in so we can detect and fix any remaining issues before putting the blocks back in.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 14 • 16 years ago

Same as above with -w

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 15 • 16 years ago

This patch removes a problem where we can potentially get into a bad state (running script while there are scriptblockers) in which certain DOM features don't work correctly and we can potentially end up in a deadlock (waiting for alert() to finish, while at the same time blocking the user from clicking on the alert).

The patch also changes this bad state so that it's much less severe in case there are other ways we can get there. So said DOM features should work as before, including the alert(). Instead we just assert if we get into this unexpected state.

Both these problems are recent regressions.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 16 • 16 years ago

I would say this is blocking as it seems like there are a lot of ways that this can cause script to run while we have scriptblockers

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 17 • 16 years ago

Comment on attachment 316127 [details] [diff] [review]
Patch to fix

a1.9=beltzner

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 18 • 16 years ago

Checked in. Thanks for the speedy review.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 19 • 16 years ago

Jonas, did you really want to do
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsGenericElement.cpp&rev=3.651&mark=3248#3246 ?

I think you meant:
 mozAutoDocConditionalContentUpdateBatch updateBatch(aDocument,

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 20 • 16 years ago

Doh! Yes. Filed bug 433421