436965 - <iframe src="c:"></iframe> triggers "ASSERTION: This is unsafe"

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

And in bug 459710 similarly.

Smaug, could we make frameloader init always async?  Or at least do it off a scriptrunner or some such?

Comment 5

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

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

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

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

Comment 8

[Daniel Veditz [:dveditz]]

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

Comment 9

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

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

Comment 10

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

Please go ahead :)

Comment 11

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

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

Comment 12

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

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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?

Comment 14

[Johnny Stenback (:jst)]

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.

Comment 16

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

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

Comment 17

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

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.

Comment 18

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

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?

Comment 19

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

Attachment: WIP, v4

note, this is just a wip. Approach v4.

Comment 20

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

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

Comment 21

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

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

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

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

Comment 24

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

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 25

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

Comment on attachment 349526 [details] [diff] [review]
WIP, v4

Not, good. This causes new assertions.

Comment 26

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

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

Comment 27

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

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

Comment 28

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

Comment on attachment 349526 [details] [diff] [review]
WIP, v4

This bug really needs bug 466057 to be fixed first.

Comment 29

[Johnny Stenback (:jst)]

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

Comment 30

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

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

Comment 31

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

Comment on attachment 349526 [details] [diff] [review]
WIP, v4

this patch doesn't pass all the tests anymore :(

Comment 32

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

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

Comment 33

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

Attachment: v5

This fixes also bug 430800.

Comment 34

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

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.

Comment 35

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

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

Comment 36

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

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

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

Attachment: v6

Comment 38

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

Comment on attachment 354960 [details] [diff] [review]
v6

Wanna add an assertion to ProcessPendingRestyles that checks that we're inside a scriptblocker?

Comment 39

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

Attachment: with assertion

Comment 40

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

http://hg.mozilla.org/mozilla-central/rev/806040db5068

Comment 41

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

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

Comment 42

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

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

Comment 43

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

http://hg.mozilla.org/mozilla-central/rev/c269ca158313

Comment 44

[Boris Zbarsky [:bzbarsky]]

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.

Comment 45

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

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.

Comment 46

[Samuel Sidler (old account; do not CC)]

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

Comment 47

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

Attachment: 1.9.0 patch

No functional changes comparing to the 1.9.1/trunk patch.

Comment 49

[Daniel Veditz [:dveditz]]

Comment on attachment 360071 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 50

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 51

[Carsten Book [:Tomcat]]

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