Loading the testcase on one of my 10.5 machines triggers: ###!!! ASSERTION: This is unsafe: 'nsContentUtils::IsSafeToRunScript()', file /Users/jruderman/central/layout/base/nsDocumentViewer.cpp, line 1054 ###!!! ASSERTION: Someone forgot to block scripts: 'aIsSafeToFlush == nsContentUtils::IsSafeToRunScript()', file /Users/jruderman/central/layout/base/nsPresShell.cpp, line 4505 ###!!! ASSERTION: How did we get here if it's not safe to run scripts?: 'nsContentUtils::IsSafeToRunScript()', file /Users/jruderman/central/layout/base/nsPresShell.cpp, line 5556

The problem is that we're putting up the silly dialog that says we can't handle the scheme... and we're doing it with this stack: #39 0x042a61b0 in nsPromptService::Alert (this=0xb5dd160, parent=0xd756120, dialogTitle=0xd790c60, text=0xb5e8f68) at /Users/bzbarsky/mozilla/profile/mozilla/embedding/components/windowwatcher/src/nsPromptService.cpp:143 #40 0x0428e956 in nsPrompt::Alert (this=0xd10c3e0, dialogTitle=0x0, text=0xb5e8f68) at /Users/bzbarsky/mozilla/profile/mozilla/embedding/components/windowwatcher/src/nsPrompt.cpp:198 #41 0x03408154 in nsDocShell::DisplayLoadError (this=0x8627a0, aError=2152398866, aURI=0x673d3e0, aURL=0x0, aFailedChannel=0x0) at /Users/bzbarsky/mozilla/profile/mozilla/docshell/base/nsDocShell.cpp:3227 #42 0x034112b5 in nsDocShell::InternalLoad (this=0x8627a0, aURI=0x673d3e0, aReferrer=0xd26e3d0, aOwner=0xd28fbf0, aFlags=0, aWindowTarget=0xd2bda60, aTypeHint=0x0, aPostData=0x0, aHeadersData=0x0, aLoadType=1, aSHEntry=0x0, aFirstParty=0, aDocShell=0x0, aRequest=0x0) at /Users/bzbarsky/mozilla/profile/mozilla/docshell/base/nsDocShell.cpp:7272 #43 0x033f9ca1 in nsDocShell::LoadURI (this=0x8627a0, aURI=0x673d3e0, aLoadInfo=0xc5b0880, aLoadFlags=0, aFirstParty=0) at /Users/bzbarsky/mozilla/profile/mozilla/docshell/base/nsDocShell.cpp:932 #44 0x01b41b80 in nsFrameLoader::ReallyStartLoading (this=0xd213b60) at /Users/bzbarsky/mozilla/profile/mozilla/content/base/src/nsFrameLoader.cpp:218 #45 0x01b2a67c in nsDocument::InitializeFrameLoader (this=0x1550800, aLoader=0xd213b60) at /Users/bzbarsky/mozilla/profile/mozilla/content/base/src/nsDocument.cpp:5082 #46 0x01b4167b in nsFrameLoader::LoadURI (this=0xd213b60, aURI=0x673d3e0) at /Users/bzbarsky/mozilla/profile/mozilla/content/base/src/nsFrameLoader.cpp:183 #47 0x01b40252 in nsFrameLoader::LoadFrame (this=0xd213b60) at /Users/bzbarsky/mozilla/profile/mozilla/content/base/src/nsFrameLoader.cpp:164 #48 0x01c10d93 in nsGenericHTMLFrameElement::LoadSrc (this=0xd15da30) at /Users/bzbarsky/mozilla/profile/mozilla/content/html/content/src/nsGenericHTMLElement.cpp:2833 #49 0x01c10f22 in nsGenericHTMLFrameElement::BindToTree (this=0xd15da30, aDocument=0x1550800, aParent=0xb58e8f0, aBindingParent=0x0, aCompileEventHandlers=1) at /Users/bzbarsky/mozilla/profile/mozilla/content/html/content/src/nsGenericHTMLElement.cpp:2856 #50 0x01b55fa0 in nsGenericElement::doInsertChildAt (aKid=0xd15da30, aIndex=0, aNotify=0, aParent=0xb58e8f0, aDocument=0x1550800, aChildArray=@0xb58e908) at /Users/bzbarsky/mozilla/profile/mozilla/content/base/src/nsGenericElement.cpp:3252 so totally inside an update. The right thing to do is probably to put off the load until after the update is over. That's generally a good thing to do, because a load start can run chrome JS no matter what, by sending necko notifications. Alternately, the posing of the error dialog could be async. Not sure which is better.

So.. This can cause us to run script in the middle of frame construction, which is easily followed by a grisly death and certainly exploitable. See bug 459710 for an example of the former.

OK, so we are NOT in fact inside an update yet, since we're just inside BindToTree from the sink...

And in bug 459710 similarly. Smaug, could we make frameloader init always async? Or at least do it off a scriptrunner or some such?

(In reply to comment #3) > OK, so we are NOT in fact inside an update yet, since we're just inside > BindToTree from the sink... Um, we're not in update when calls are coming from sink. Evil.

Well, technically updates are for the mutation observer notifications. The more I think about this, the more I think that doing the load sync under BindToTree is a bad idea and that that's what should happen either async or at end of update or at end of script blocking or something... I'm not sure the sink puts in a script blocker either, fwiw.

(In reply to comment #6) > The more I think about this, the more I think that doing the load sync under > BindToTree is a bad idea and that that's what should happen either async or at > end of update Loading happens at the end of update, if there is update. Async may change DOM event ordering etc. too much - even moving loading to happen after update caused bad regressions.

Given the short cycle for 1.9.0.5 this probably has to wait.

I can take this. Unless you Jonas have already something ready for this.

Please go ahead :)

(In reply to comment #6) > The more I think about this, the more I think that doing the load sync under > BindToTree is a bad idea and that that's what should happen either async or at > end of update or at end of script blocking or something... I'm not sure the > sink puts in a script blocker either, fwiw. Sync load in BindToTree is bad, sure. I've tried to make async+after update working, but no luck yet. The ordering of stuff gets tricky. Sink doesn't seem to push any script blockers.

Feel free to add scriptblocker anyplace you think is needed. The current set is by no means exhaustive.

Adding scriptblockers around every bindtotree in the sink might be kinda nice, but are we really happy doing the load as soon as we return to the sink?

Bumping priority and marking a blocker, we should fix this security bug for 1.9.1. Smaug, are you able to continue working on this? If not, please let me know.

(In reply to comment #14) > Smaug, are you able to continue working on this? Yeah, still working on this.

Note, (X)HTML is the easy part here, but making XUL and XBL work nicely... I do have a patch which should pass all the mochi/chrome/browser-chrome tests, but I think the tests don't cover dynamic overlays and it seems we don't have good tests for <frame> elements - most of the tests use <iframe>s. The idea is anyway to delay frame loading (when there are no document updates). Asynchronous loading isn't possible, that breaks lots of testcases.

What things fail if we delay loading? Is there any chance that we can fix those longer term? Is the problem only that we're putting up the dialog? If so, maybe we can just ensure that the dialog is coming up async?

note, this is just a wip. Approach v4.

(In reply to comment #18) > What things fail if we delay loading? Is there any chance that we can fix those > longer term? about:blank, for example, must load ASAP. Otherwise all sort of cases, where iframe.contentDocument is expected to be available, will fail. I need to verify this fixes all the things I want. Running mochitest asserts way too much about script blockers - hard to say which assertions are because of this bug and which are because of something else. Would be great to get the patches for bug 430214 in soon. That might reduce the noise at least a bit.

(In reply to comment #18) > Is the problem only that we're putting up the dialog? If so, maybe we can just > ensure that the dialog is coming up async? That might be ok for some cases, but *maybe* not enough if there is |load| event listener for the frame.

Does the about:blank really need to start loading ASAP? Or just to have an document viewer ASAP?

The load event must be fired for about:blank asap, so no, I don't think having just document viewer is enough.

I think for <iframe> loading can be delayed up to DoneAddingChildren, but because <frame> is a leaf, loading can't be delayed much longer than what the patch does.

Comment on attachment 349526 [details] [diff] [review] WIP, v4 Not, good. This causes new assertions.

I think bug 444224 must be fixed first to reduce assertion noise.

If bug 466057 is fixed, the patch may make sense after all.

Comment on attachment 349526 [details] [diff] [review] WIP, v4 This bug really needs bug 466057 to be fixed first.

Smaug, looks like bug 466057 is fixed now, can you make more progress here now, or are there other things blocking this as well?

Yes, I'll re-test the patch and ask reviews if I still think it is ok.

Comment on attachment 349526 [details] [diff] [review] WIP, v4 this patch doesn't pass all the tests anymore :(

(In reply to comment #31) > (From update of attachment 349526 [details] [diff] [review]) > this patch doesn't pass all the tests anymore :( Something to do with Bug 345339. I'll look at the problem tomorrow.

This fixes also bug 430800.

Comment on attachment 354689 [details] [diff] [review] v5 > nsDocument::InitializeFrameLoader(nsFrameLoader* aLoader) > nsDocument::FinalizeFrameLoader(nsFrameLoader* aLoader) > nsDocument::InitializeFinalizeFrameLoaders() Do we need these functions at all? Wouldn't it be cleaner to simply issue a script runner for each frame that needs to be initialized/finalized that just deals with that one initialization/finalization. >diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp >@@ -13559,16 +13559,17 @@ nsCSSFrameConstructor::LazyGenerateChild > nsMenuPopupFrame* menuPopupFrame = static_cast<nsMenuPopupFrame *>(frame); > if (menuPopupFrame->HasGeneratedChildren()) { > if (mCallback) > mCallback(mContent, frame, mArg); > > return NS_OK; > } > >+ nsAutoScriptBlocker scriptBlocker; > // indicate that the children have been generated > menuPopupFrame->SetGeneratedChildren(); > #endif Seems scary to have a scriptblocker that's #ifdef MOZ_XUL, but whose lifetime spans more than the ifdef. Also, it currently is alive while ProcessAttachedQueue is called further down which is bad since ProcessAttachedQueues purpose is to run scripts. >diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp >--- a/layout/base/nsPresShell.cpp >+++ b/layout/base/nsPresShell.cpp >@@ -2403,17 +2403,20 @@ PresShell::InitialReflow(nscoord aWidth, > // Run the XBL binding constructors for any new frames we've constructed > mDocument->BindingManager()->ProcessAttachedQueue(); > > // Constructors may have killed us too > NS_ENSURE_STATE(!mHaveShutDown); > > // Now flush out pending restyles before we actually reflow, in > // case XBL constructors changed styles somewhere. >- mFrameConstructor->ProcessPendingRestyles(); >+ { >+ nsAutoScriptBlocker scriptBlocker; >+ mFrameConstructor->ProcessPendingRestyles(); >+ } Why is this needed? Not sure what ProcessPendingRestyles ends up doing so mostly asking out of curiosity. (same at the other callsites). Does it bind the anonymous content which schedules the iframe scriptrunners? Minusing based on the nsCSSFrameConstructor::LazyGenerateChild change.

(In reply to comment #34) > (From update of attachment 354689 [details] [diff] [review]) > > nsDocument::InitializeFrameLoader(nsFrameLoader* aLoader) > > nsDocument::FinalizeFrameLoader(nsFrameLoader* aLoader) > > nsDocument::InitializeFinalizeFrameLoaders() Unfortunately document (or something) needs to keep track on to-be-initialized frameloaders, because docshell may want to cancel initialization or docshell may want to ask if some frameloader is scheduled to be finalized. > Seems scary to have a scriptblocker that's #ifdef MOZ_XUL, but whose lifetime > spans more than the ifdef. Oops. > > Also, it currently is alive while ProcessAttachedQueue is called further down > which is bad since ProcessAttachedQueues purpose is to run scripts. Ah, right. > Does it bind the anonymous content which schedules the iframe scriptrunners? For example that yes.

(In reply to comment #35) > (In reply to comment #34) > > (From update of attachment 354689 [details] [diff] [review] [details]) > > > nsDocument::InitializeFrameLoader(nsFrameLoader* aLoader) > > > nsDocument::FinalizeFrameLoader(nsFrameLoader* aLoader) > > > nsDocument::InitializeFinalizeFrameLoaders() > Unfortunately document (or something) needs to keep track on to-be-initialized > frameloaders, because docshell may want to cancel initialization or > docshell may want to ask if some frameloader is scheduled to be finalized. The to-be-initialized information could be kept in the iframes docshell (for example by making it point to the scriptrunner), that'd also simplify the cancel-pending-initialization code. Not sure about the schedule-to-be-finalized part though, though that looks messy anyway. Anyway, I'm fine with leaving this as is given the complexity of rewriting, but it'd be nice to do at some point perhaps.

Comment on attachment 354960 [details] [diff] [review] v6 Wanna add an assertion to ProcessPendingRestyles that checks that we're inside a scriptblocker?

Backed out because of some OSX pixel scrolling failures (which I can't reproduce locally on OSX nor on Linux).

The test failure has occurred also before. Going to re-land.

I assume this did land on 1.9.1? Also, can we land this on 1.9.0? That would be really nice for bug 453736.

Yes, this landed 1.9.1. (there is fixed1.9.1). I forgot the hg link. This depends on bug 466057, so if we take this to 1.9.0, also that one is needed.

Uh, yeah. Olli, let's get both this bug and bug 466057 on the 1.9.0 branch. Can you work up a couple patches (or a combined one, whichever is better/easier)?

No functional changes comparing to the 1.9.1/trunk patch.

Comment on attachment 360071 [details] [diff] [review] 1.9.0 patch Approved for 1.9.0.7, a=dveditz for release-drivers.

Tomcat, this is another of those where I need a debug build in order to use the testcase. Can you verify this fix on OS X 10.5 with debug 1.9.0.7 nightly?

verified 1.9.0.7 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8pre) Gecko/2009021822 Firefox/3.0.8pre - have not seen this assertion on the testcase --> verified