596765 - SVG foreignObject never renders if its document is initially zero-sized

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

roc just pointed out to me that we could very well have plugins & iframes inside of <foreignObject> tags in SVG images, and that could provide a way to get scripts run from an <img> tag or a CSS background. (which is bad)

To fix this, we need to make sure plugins & iframes are disabled within SVG-as-an-image.

Comment 1

[Boris Zbarsky [:bzbarsky]]

You probably want to just change the check at http://hg.mozilla.org/mozilla-central/file/f06a18912bb4/content/base/src/nsDataDocumentContentPolicy.cpp#l87 from !doc->GetDisplayDocument() to IsResourceDoc(), right?

Comment 2

[Daniel Holbert [:dholbert]]

Yeah, looks like it -- thanks for the tip!

Comment 3

[Boris Zbarsky [:bzbarsky]]

You're welcome.  Sorta feels like it's my fault that the check is in that slightly odd place.  ;)

Comment 4

[Daniel Holbert [:dholbert]]

Actually, in working up a testcases for this, I've discovered that <foreignObject> doesn't render at all in SVG-as-an-image, right now.  I'm morphing this bug into making that work. (with the check that bz suggested in comment 1, to be sure it's safe)

We'll also want to be sure that links in a <foreignObject> always get printed with the not-visited color, to be sure we don't leak browsing-history.  (This is already fine for SVG links in SVG-as-an-image -- those are displayed without any :visited/:link coloring at all right now, so they don't leak information.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: SVG for testcase 1

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (should show text in XHTML, but not in iframe) [broken in Opera]

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: SVG for testcase 2

Opera 10.62 doesn't display the <foreignObject> in the first testcase's SVG at all (even when I directly view the SVG), apparently because the foreignObject is in a translated <g> element.

I'm attaching a second testcase with no translated-<g>, to get Opera's behavior.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Comment 9

[Daniel Holbert [:dholbert]]

FWIW, Opera 10.62 actually shows the text in the iframe -- except there seems to be a race condition where it sometimes doesn't show the text, if the HTML & SVG is all on my local machine. (In that case, the text in the iframe only shows up the first time I load the testcase.)

Comment 10

[Daniel Holbert [:dholbert]]

Ok -- so the first problem here is:
 - The first time we reflow our SVG document, its viewport has width & height = 0, which traces back to this line of SVGDocumentWrapper:
253     rv = mViewer->Init(nsnull, nsIntRect(0, 0, 0, 0));
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/SVGDocumentWrapper.cpp#253

(That's because we haven't drawn anything yet, so we don't know what size our viewport will actually end up being.)

 - This makes our nsSVGForeignObjectFrame start out by caching a zero-filled mCanvasTM and setting its mRect to have height & width = 0. That happens inside of nsSVGForeignObjectFrame::UpdateCoveredRegion() (which calls GetCanvasTM() to cache the mCanvasTM)

 - As a result, our initial nsSVGForeignObjectFrame::DoReflow call fails its initial check on IsDisabled() (which really just checks for a zero-size mRect).  So our foreignObject frame never reflows.

 - Later, when we hit VectorImage::Draw and make our document nonzero-size, we've still got two things that block our nsSVGForeignObjectFrame from reflowing:
    * Our kid isn't marked as dirty, so we return early from nsSVGForeignObjectFrame::MaybeReflowFromOuterSVGFrame.
    * nsSVGForeignObjectFrame::mCanvasTM is still cached & filled with zeroes, so subsequent calls to UpdateCoveredRegion will use that and keep using a zero-sized mRect.  (So even if we did make it into DoReflow, we'd end up failing the "IsDisabled" check.  And even if we passed that, we still have a zero-sized mRect, so nothing would show up.)

WORKAROUND: If I change SVGDocumentWrapper.cpp:253 to use a nonzero height/width (like e.g. 100/100), then I see the XHTML text painted in both testcases here, and nothing painted in the iframe.  (the behavior that we want).

I'd prefer to keep SVGDocumentWrapper.cpp:253 as-is, though (rather than imposing an arbitrary initial height/width on our helper-document) and fix the bug with nsSVGForeignObjectFrame getting locked at zero-size.

Comment 11

[Joe Drew (not getting mail)]

Would approve a patch!

Comment 12

[Daniel Holbert [:dholbert]]

Been looking at this a bit more.

Turns out Comment 10 isn't entirely correct.  What actually happens is more like this:

 (a) Our nsSVGForeignObjectFrame and its nsBlockFrame both start out with NS_FRAME_FIRST_REFLOW & NS_FRAME_IS_DIRTY. (because they've never yet been reflowed)

 (b) We hit nsSVGForeignObjectFrame::InitialUpdate
   (1) InitialUpdate calls DoReflow(), but that's a no-op - it takes the "IsDisabled()" early-return, since we're zero-sized.
   (2) InitialUpdate then clears our NS_FRAME_IS_DIRTY bit.
 (c) MEANWHILE: our child (a nsBlockFrame*) has never been reflowed, and its NS_FRAME_IS_DIRTY bit is still set.  (This is important!!)

 (d) Later on, after we're a non-zero size, we get a call to MaybeReflowFromOuterSVGFrame().  However -- that ends up being a no-op (returning early, before the DoReflow call), because it sees that our child has its NS_FRAME_IS_DIRTY bit set, and it mistakenly thinks that there's already a pending reflow for our child.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: fix v1

(In reply to comment #12)
>  (b) We hit nsSVGForeignObjectFrame::InitialUpdate
>    (1) InitialUpdate calls DoReflow(), but that's a no-op - it takes the
> "IsDisabled()" early-return, since we're zero-sized.

This is one point at which we can fix this bug.

It seems like for our initial reflow, it'd be worth bypassing the "IsDisabled" check, so that we get all of our children's state bits in a consistent state.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (standalone SVG)

This testcase demonstrates that this is a problem for <foreignObject> in standalone SVG, too.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: reference for testcase 3

This reference only differs from testcase 3 in that it synchronously tweaks height/width, rather than doing so asynchronously.  (This makes them nonzero for the initial reflow, thereby avoiding this bug.)

Comment 16

[Daniel Holbert [:dholbert]]

Hm, looks like the IsDisabled() check in DoReflow was added by bug 385246:
http://hg.mozilla.org/mozilla-central/diff/b6d2bf714f9f/layout/svg/base/src/nsSVGForeignObjectFrame.cpp

So this seems to have been caused by that bug.  And the current patch here actually regresses that bug (makes us fail assertions on its testcase).  So the current patch isn't quite what we want.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v2

Ok -- so the previous patch would failing this assertion
> nsSVGForeignObjectFrame::DoReflow()
> {
>   nsSize size(nsPresContext::CSSPixelsToAppUnits(width),
>               nsPresContext::CSSPixelsToAppUnits(height));
[...]
>   NS_ASSERTION(size.width == desiredSize.width &&
>                size.height == desiredSize.height, "unexpected size");
whenever the height or width attr was negative, because we clamp them to non-negative values in nsSVGForeignObjectFrame::UpdateCoveredRegion, but we don't do that here.

So, I've just added a NS_MAX() call, to clamp here as well.

Comment 18

[Daniel Holbert [:dholbert]]

sorry, s/would failing/would fail/

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: patch 2: Also return early from MaybeReflowFromOuterSVGFrame

One of the reftests that I wrote for this was still failing.  It turned out it was because we'd hit a related problem to what I described in Comment 12, on the *second* (or later) reflow.

Basically: if we get a reflow (after our initial one) from MaybeReflowFromOuterSVGFrame while we're still zero-sized, then:
 (a) MaybeReflowFromOuterSVGFrame marks our child as dirty
 (b) MaybeReflowFromOuterSVGFrame calls DoReflow
 (c) DoReflow returns early (not reflowing anything), because we're zero-sized.

That leaves us with a stale dirty-marking on our child, which makes all future calls to MaybeReflowFromOuterSVGFrame return early since they (mistakenly) think that the dirty-marking means we've already got a reflow queued up.

This patch fixes that by adding an early-return before the dirty-marking.

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: patch 2: Also return early from MaybeReflowFromOuterSVGFrame

(forgot to qref patch 2 after tweaking its explanatory comment - fixed here.)

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: patch 3: Skip 'ignoreViewportScrolling' chunk of nsLayoutUtils::PaintFrame for non-root frames

This final tweak fixes the following assertion-failure...
> ###!!! ASSERTION: must have root frame: '!aFrame->GetParent()', file ../../../mozilla/layout/base/nsLayoutUtils.cpp, line 1402
...that we otherwise would fail in SVG-as-an-image testcases that use <foreignObject>. (after this bug's first patch is applied)

We fail this assertion because:
 - we happen to use nsLayoutUtils::PaintFrame to render <foreignObject>
 - nsLayoutUtils::PaintFrame assumes "scrolling disabled" means "I'm reflowing the root frame"
 - SVG-as-an-image happens to have scrolling disabled

This patch just softens the assumption and makes us explicitly check if we've got the root frame before we enter the clause in question.  (I think this is correct, because the clause in question was added as part of merging RenderDocument into nsLayoutUtils::PaintFrame* -- which I think means that it's only important for root frames.)

* bug 564991 comment 19

Comment 22

[Daniel Holbert [:dholbert]]

Attachment: patch 4: tests

Here are some reftests for this.

Notes on tests:
* layout/reftests/svg/foreignObject-start-hidden-01.svg
  - This test *fails* with only patch 1 applied, but patch 2 fixes it.

* layout/reftests/svg/foreignObject-start-hidden-02.svg
  - This test passes with only patch 1 applied.  It only differs from the previous test in that it's got a viewBox  (plus pAR=none to make it fill the whole screen).  (Apparently the viewBox gets us an additional notification and works around the issue that's addressed in patch 2)

* layout/reftests/svg/as-image/img-foreignObject-1.html
  - This test visually passes with only patch 1 applied, but it needs patch 3 to keep from failing an assertion.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So is there going to be a patch here that does comment #1?

Comment 24

[Daniel Holbert [:dholbert]]

As it turns out, we actually already did a variant of comment 1, in bug 628747. :)

Comment 25

[Daniel Holbert [:dholbert]]

Landed:
 http://hg.mozilla.org/mozilla-central/rev/7754be6aecc9
 http://hg.mozilla.org/mozilla-central/rev/680f69bed54d
 http://hg.mozilla.org/mozilla-central/rev/762e8424548c
 http://hg.mozilla.org/mozilla-central/rev/58cfb5629c44

Added one reftest that uses an iframe with text, a red background, and an alert() popup, all inside of a <foreignObject> element in a SVG file that gets used as an image.  The iframe's contents (and its 'alert') show up when I view the SVG directly, but not when I view it as an image.

Comment 26

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> roc just pointed out to me that we could very well have plugins & iframes
> inside of <foreignObject> tags in SVG images, and that could provide a way to
> get scripts run from an <img> tag or a CSS background. (which is bad)

In sanity-checking this: so now that we render <foreignObject> (after the landings in comment 25), it looks like we do actually instantiate plugins, but we don't let them execute or render anything.

I get this output when I view a <embed ... src="something.wav"> in foreignObject-inside-SVG-as-an-image:
> For audio/wav found plugin libtotem-cone-plugin.so
> ** (<unknown>:32052): DEBUG: totemPlugin [0x7ff3741f3140]
> ** (<unknown>:32052): DEBUG: 0x7ff3741f3140: "Init mimetype 'audio/wav' mode 1"
> WARNING: NS_ENSURE_TRUE(sgo) failed: file mozilla/modules/plugin/base/src/nsNPAPIPlugin.cpp, line 737
> WARNING: NS_ENSURE_TRUE(cx) failed: file mozilla/modules/plugin/base/src/nsNPAPIPlugin.cpp, line 1350
> WARNING: NS_ENSURE_TRUE(sgo) failed: file mozilla/modules/plugin/base/src/nsNPAPIPlugin.cpp, line 737
> WARNING: NS_ENSURE_TRUE(cx) failed: file ../../../../../mozilla/modules/plugin/base/src/nsNPAPIPlugin.cpp, line 1350

(those warnings are for failures to obtain a nsIScriptGlobalObject and a JSContext)

So, plugins don't have all their normal power -- but it's still best to intercept them before we've even instantiated them.  One-liner followup coming up to do that.

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: followup to disable plugin instantiation

This adds an early return to the method that makes us instantiate plugins.

Comment 28

[Daniel Holbert [:dholbert]]

> This adds an early return
(or rather, it extends an existing early-return for static documents (e.g. printouts), to make that early-return cover svg-as-an-image as well.)

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 513338 [details] [diff] [review]
followup to disable plugin instantiation

needs a test

Comment 30

[Daniel Holbert [:dholbert]]

Test coming up.

Comment 31

[Daniel Holbert [:dholbert]]

Attachment: reftest for plugins

Here's a reftest with a plugin.  Before the followup patch is applied, the included HTML testcase renders with "download plugin" UI inside of the image, some low but nonzero fraction of the time.  (and it renders that way 100% of the time if I view the svg directly, which is expected)

Comment 32

[Daniel Holbert [:dholbert]]

Landed followup with reftest:
 http://hg.mozilla.org/mozilla-central/rev/911ef52fbc49

Comment 33

[Daniel Holbert [:dholbert]]

Darn - we apparently still occasionally show the "download plugin" UI inside of the image some nonzero fraction of the time after the followup-patch, too -- the reftest failed on Linux64 opt & Win7 debug:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1298012309.1298012955.1833.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1298010623.1298013764.4601.gz

I disabled the reftest for now:
  http://hg.mozilla.org/mozilla-central/rev/1eed87fac9ce
Left the patch in, since I'm still pretty sure it does what we want, given that it reliably silences the terminal output from Comment 26.  Ideally I'd like to write a test using a plugin that e.g. crashes as soon as it's instantiated - not I'm sure if that's already available or how easy that would be to write.

Tomorrow I'll look into fixing the test or making a better test, and I'll also look into what's going on with the "download plugin" UI still showing up, despite plugin-instantiation being disabled. (maybe those two aren't connected?)

Comment 34

[Daniel Holbert [:dholbert]]

From searching for calls to "doc->IsLoadedAsData()" and "doc->IsStaticDocument()" (two "similar" conditions to image-ness), it looks like I might need to add one more early-return in doc->IsBeingUsedAsImage().

Seems to work locally.  Running it through TryServer, & will report back after.

Comment 35

[Daniel Holbert [:dholbert]]

(In reply to comment #34)
> need to add one more early-return in doc->IsBeingUsedAsImage().
er, I meant "one more early-return in nsObjectLoadingContent::LoadObject"

Comment 36

[Daniel Holbert [:dholbert]]

Attachment: followup #2: Return early from LoadObject if we're an image document

Looks good on TryServer so far: a Try batch without this fix has failed the reftest on 2 of 6 completed runs, whereas a Try batch *with* the fix has completed 8 reftest runs successfully with no failures yet.

(I was originally going to group this with the existing "doc->IsLoadedAsData()" / "doc->IsStaticDocument()" checks, a few lines further down, but based on the contextual code & comment, it looks like those checks are asking "Does our doc have a URI that we can check against our security policy?" -- so this patch doesn't logically belong with those.  So, I just put the check in as early as possible -- right after we get a handle for our nsIDocument.

Comment 37

[Daniel Holbert [:dholbert]]

Landed followup #2: http://hg.mozilla.org/mozilla-central/rev/657c2a92ee2b

Comment 38

[Daniel Holbert [:dholbert]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23)
> So is there going to be a patch here that does comment #1?
(In reply to Daniel Holbert [:dholbert] from comment #24)
> As it turns out, we actually already did a variant of comment 1, in bug
> 628747. :)

Just to clarify this -- so bug 628747 prevented <iframes> that point to *external* resources, but didn't address <iframes src="data URI">. (an it's possible that, if allowed, this sort of <iframe> might help get around other restrictions, as Jesse suggested at http://robert.ocallahan.org/2011/11/drawing-dom-content-to-canvas.html?showComment=1320734103970#c4021554625723456207 )

I looked into this more thoroughly today, to be sure we block <iframes> even when they point to data URIs, and we do, albeit somewhat accidentally.

This is where we block them (line 1387):
> 1360 nsresult
> 1361 nsFrameLoader::MaybeCreateDocShell()
> 1362 {
[...]
> 1384   if (doc->GetDisplayDocument() || !doc->IsActive()) {
> 1385     // Don't allow subframe loads in external reference documents, nor
> 1386     // in non-active documents.
> 1387     return NS_ERROR_NOT_AVAILABLE;
> 1388   }
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1360

For SVG-as-an-image, the doc->IsActive() check there returns false (getting us to the early return), because it checks doc->mDocumentContainer which is null.

Also, even if that early-return weren't there, we'd still be OK, because the next few lines try to get & use doc->mDocumentContainer, and we end up bailing out because (again) mDocumentContainer is null.

So we're OK on this.  Still, we should probably apply s/GetDisplayDocument()/IsResourceDoc()/ there for good measure & clarity.

Comment 39

[Daniel Holbert [:dholbert]]

Also, BTW, we already of have a reftest for <iframe src="data URI"> though it doesn't directly test rendering -- it tests scripting.  That test is img-foreignObject-embed-1.html

I'll tweak that reftest so that it tests rendering (it almost does already), and make the substitution that I suggested at the end of my previous comment.  I'll do that over in bug 700895, which I just filed.