Last Comment Bug 436965 - <iframe src="c:"></iframe> triggers "ASSERTION: This is unsafe"
: <iframe src="c:"></iframe> triggers "ASSERTION: This is unsafe"
Status: RESOLVED FIXED
: [sg:critical] post 1.8-branch
: assertion, fixed1.9.1, testcase, verified1.9.0.7
Product: Core
Classification: Components
Component: Layout: HTML Frames
: Trunk
: x86 Mac OS X
: P1 normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: layout.html-frames
: http://ag.arizona.edu/azaqua/ista/Rec...
: 466057 472502
: 444224 467422 430800 453736 459710
  Show dependency treegraph
 
Reported: 2008-06-02 17:49 PDT by Jesse Ruderman
Modified: 2009-03-12 22:42 PDT (History)
15 users (show)
jst: blocking1.9.1+
jonas: wanted1.9.1+
dveditz: blocking1.9.0.7+
dveditz: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
See Also:
Crash Signature:


Attachments
testcase (27 bytes, text/html)
2008-06-02 17:49 PDT, Jesse Ruderman
no flags Details
WIP, v4 (12.17 KB, patch)
2008-11-21 17:04 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v5 (10.53 KB, patch)
2008-12-29 05:16 PST, Olli Pettay [:smaug]
jonas: review-
Details | Diff | Splinter Review
v6 (11.60 KB, patch)
2008-12-31 08:18 PST, Olli Pettay [:smaug]
jonas: review+
jonas: superreview+
Details | Diff | Splinter Review
with assertion (12.25 KB, patch)
2009-01-02 04:40 PST, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
1.9.0 patch (13.54 KB, patch)
2009-02-02 05:02 PST, Olli Pettay [:smaug]
dveditz: approval1.9.0.7+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2008-06-02 17:49:51 PDT
Created attachment 323474 [details]
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 (:bz) 2008-10-09 13:12:20 PDT
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 (:bz) 2008-10-21 11:24:28 PDT
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 (:bz) 2008-10-21 11:41:23 PDT
OK, so we are NOT in fact inside an update yet, since we're just inside BindToTree from the sink...
Comment 4 Boris Zbarsky (:bz) 2008-10-21 11:44:00 PDT
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] 2008-10-21 15:40:38 PDT
(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 (:bz) 2008-10-21 17:28:09 PDT
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] 2008-10-22 01:19:53 PDT
(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 2008-10-31 11:55:20 PDT
Given the short cycle for 1.9.0.5 this probably has to wait.
Comment 9 Olli Pettay [:smaug] 2008-11-13 16:48:31 PST
I can take this. Unless you Jonas have already something ready for this.
Comment 10 Jonas Sicking (:sicking) 2008-11-13 17:00:36 PST
Please go ahead :)
Comment 11 Olli Pettay [:smaug] 2008-11-18 14:04:47 PST
(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) 2008-11-18 14:12:37 PST
Feel free to add scriptblocker anyplace you think is needed. The current set is by no means exhaustive.
Comment 13 Boris Zbarsky (:bz) 2008-11-18 14:21:46 PST
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, jst@mozilla.com) 2008-11-20 16:35:37 PST
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 15 Johnny Stenback (:jst, jst@mozilla.com) 2008-11-20 16:36:17 PST
*** Bug 459710 has been marked as a duplicate of this bug. ***
Comment 16 Olli Pettay [:smaug] 2008-11-21 03:57:16 PST
(In reply to comment #14)
> Smaug, are you able to continue working on this?
Yeah, still working on this.
Comment 17 Olli Pettay [:smaug] 2008-11-21 13:47:06 PST
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) 2008-11-21 16:40:32 PST
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] 2008-11-21 17:04:15 PST
Created attachment 349526 [details] [review]
WIP, v4

note, this is just a wip. Approach v4.
Comment 20 Olli Pettay [:smaug] 2008-11-21 17:27:49 PST
(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] 2008-11-21 17:29:49 PST
(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 (:bz) 2008-11-21 18:37:10 PST
Does the about:blank really need to start loading ASAP?  Or just to have an document viewer ASAP?
Comment 23 Olli Pettay [:smaug] 2008-11-22 00:20:11 PST
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] 2008-11-22 00:36:25 PST
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] 2008-11-22 15:07:58 PST
Comment on attachment 349526 [details] [review]
WIP, v4

Not, good. This causes new assertions.
Comment 26 Olli Pettay [:smaug] 2008-11-22 15:11:19 PST
I think bug 444224 must be fixed first to reduce assertion noise.
Comment 27 Olli Pettay [:smaug] 2008-11-26 07:08:43 PST
If bug 466057 is fixed, the patch may make sense after all.
Comment 28 Olli Pettay [:smaug] 2008-12-04 06:27:54 PST
Comment on attachment 349526 [details] [review]
WIP, v4

This bug really needs bug 466057 to be fixed first.
Comment 29 Johnny Stenback (:jst, jst@mozilla.com) 2008-12-22 12:43:15 PST
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] 2008-12-22 12:51:52 PST
Yes, I'll re-test the patch and ask reviews if I still think it is ok.
Comment 31 Olli Pettay [:smaug] 2008-12-22 17:31:41 PST
Comment on attachment 349526 [details] [review]
WIP, v4

this patch doesn't pass all the tests anymore :(
Comment 32 Olli Pettay [:smaug] 2008-12-22 17:35:40 PST
(In reply to comment #31)
> (From update of attachment 349526 [details] [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] 2008-12-29 05:16:59 PST
Created attachment 354689 [details] [review]
v5

This fixes also bug 430800.
Comment 34 Jonas Sicking (:sicking) 2008-12-30 15:30:11 PST
Comment on attachment 354689 [details] [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] 2008-12-30 15:38:27 PST
(In reply to comment #34)
> (From update of attachment 354689 [details] [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) 2008-12-30 16:47:34 PST
(In reply to comment #35)
> (In reply to comment #34)
> > (From update of attachment 354689 [details] [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] 2008-12-31 08:18:27 PST
Created attachment 354960 [details] [review]
v6
Comment 38 Jonas Sicking (:sicking) 2009-01-01 09:31:43 PST
Comment on attachment 354960 [details] [review]
v6

Wanna add an assertion to ProcessPendingRestyles that checks that we're inside a scriptblocker?
Comment 39 Olli Pettay [:smaug] 2009-01-02 04:40:11 PST
Created attachment 355101 [details] [review]
with assertion
Comment 40 Olli Pettay [:smaug] 2009-01-02 04:44:02 PST
http://hg.mozilla.org/mozilla-central/rev/806040db5068
Comment 41 Olli Pettay [:smaug] 2009-01-02 06:34:30 PST
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] 2009-01-02 07:29:33 PST
The test failure has occurred also before. Going to re-land.
Comment 43 Olli Pettay [:smaug] 2009-01-02 07:42:40 PST
http://hg.mozilla.org/mozilla-central/rev/c269ca158313
Comment 44 Boris Zbarsky (:bz) 2009-01-13 18:58:05 PST
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] 2009-01-14 00:56:47 PST
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) 2009-01-15 01:14:14 PST
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] 2009-02-02 05:02:25 PST
Created attachment 360071 [details] [review]
1.9.0 patch

No functional changes comparing to the 1.9.1/trunk patch.
Comment 49 Daniel Veditz 2009-02-02 14:43:57 PST
Comment on attachment 360071 [details] [review]
1.9.0 patch

Approved for 1.9.0.7, a=dveditz for release-drivers.
Comment 50 Al Billings [:abillings] 2009-02-13 12:13:27 PST
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] 2009-02-18 13:56:11 PST
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

Note You need to log in before you can comment on or make changes to this bug.