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

RESOLVED FIXED in mozilla2.0b12

Status

()

defect
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla2.0b12
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 -, status2.0 wanted)

Details

Attachments

(13 attachments, 2 obsolete attachments)

584 bytes, image/svg+xml
Details
188 bytes, text/html
Details
561 bytes, image/svg+xml
Details
178 bytes, text/html
Details
476 bytes, image/svg+xml
Details
459 bytes, image/svg+xml
Details
1.85 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.33 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.55 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.50 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.16 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.70 KB, patch
Details | Diff | Splinter Review
2.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
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.
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?
Yeah, looks like it -- thanks for the tip!
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
You're welcome.  Sorta feels like it's my fault that the check is in that slightly odd place.  ;)
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.)
Summary: Disable plugins & iframes in SVG when it's being used as an image → Allow foreignObject in svg-as-image (but disable plugins & iframes)
Posted image SVG for testcase 1
Attachment #475992 - Attachment description: testcase 1 (Needs to show the text in XHTML, but not in the iframe) → testcase 1 (should show text in XHTML, but not in iframe)
Posted image 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.
Attachment #475990 - Attachment description: SVG content for testcase → SVG for testcase 1
Posted file testcase 2
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.)
Attachment #475992 - Attachment description: testcase 1 (should show text in XHTML, but not in iframe) → testcase 1 (should show text in XHTML, but not in iframe) [broken in Opera]
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.
blocking2.0: --- → ?
Would approve a patch!
blocking2.0: ? → -
status2.0: --- → wanted
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.
Posted patch fix v1 (obsolete) — Splinter Review
(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.
Attachment #512988 - Flags: review?(roc)
Component: ImageLib → SVG
QA Contact: imagelib → general
This testcase demonstrates that this is a problem for <foreignObject> in standalone SVG, too.
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.)
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.
Blocks: 385246
No longer blocks: 276431
Attachment #512988 - Flags: review?(roc)
Posted patch patch 1 v2Splinter Review
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.
Attachment #512988 - Attachment is obsolete: true
Attachment #513004 - Flags: review?(roc)
sorry, s/would failing/would fail/
Summary: Allow foreignObject in svg-as-image (but disable plugins & iframes) → SVG foreignObject never renders if its document is initially zero-sized
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.
Attachment #513267 - Flags: review?(roc)
Attachment #513004 - Attachment description: fix v2 → patch 1 v2
(forgot to qref patch 2 after tweaking its explanatory comment - fixed here.)
Attachment #513267 - Attachment is obsolete: true
Attachment #513271 - Flags: review?(roc)
Attachment #513267 - Flags: review?(roc)
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
Attachment #513275 - Flags: review?(roc)
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.
Attachment #513280 - Flags: review?(roc)
So is there going to be a patch here that does comment #1?
As it turns out, we actually already did a variant of comment 1, in bug 628747. :)
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
(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.
This adds an early return to the method that makes us instantiate plugins.
Attachment #513338 - Flags: review?(roc)
> 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.)
Attachment #513338 - Flags: approval2.0?
Comment on attachment 513338 [details] [diff] [review]
followup to disable plugin instantiation

needs a test
Attachment #513338 - Flags: review?(roc)
Attachment #513338 - Flags: review+
Attachment #513338 - Flags: approval2.0?
Attachment #513338 - Flags: approval2.0+
Test coming up.
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)
Landed followup with reftest:
 http://hg.mozilla.org/mozilla-central/rev/911ef52fbc49
Flags: in-testsuite+
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?)
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.
(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"
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.
Attachment #513604 - Flags: review?(roc)
Attachment #513604 - Flags: approval2.0?
Blocks: 386690
(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.
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.
Blocks: 700895
Blocks: 601659
You need to log in before you can comment on or make changes to this bug.