Last Comment Bug 700895 - Check for IsResourceDoc() instead of GetDisplayDocument() in nsFrameLoader::MaybeCreateDocShell(), to more explicitly disable <iframes> in SVG-as-an-image
: Check for IsResourceDoc() instead of GetDisplayDocument() in nsFrameLoader::M...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on: 596765
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-08 18:29 PST by Daniel Holbert [:dholbert]
Modified: 2012-02-01 13:58 PST (History)
2 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (7.64 KB, patch)
2011-11-08 19:42 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
patch 1 v1 (7.65 KB, patch)
2011-11-08 19:47 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
patch 2 v1 (just a file rename) (2.29 KB, patch)
2011-11-08 19:52 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review
patch 3 v1: s/GetDisplayDocument/IsResourceDoc/ in ShouldLoad (1.49 KB, patch)
2011-11-12 21:33 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
patch 3 v2: s/GetDisplayDocument/IsResourceDoc/ in ShouldLoad (1.33 KB, patch)
2011-11-12 21:44 PST, Daniel Holbert [:dholbert]
roc: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-11-08 18:29:23 PST
Filing this bug on applying s/GetDisplayDocument()/IsResourceDoc()/ to  the early return in nsFrameLoader::MaybeCreateDocShell, so that check will catch SVG-as-an-image.

(Right now we inadvertantly catch SVG-as-an-image through an IsActive() check, but we should check more clearly/explicitly.)

See bug 596765 comment 38 for details.
Comment 1 Daniel Holbert [:dholbert] 2011-11-08 19:42:57 PST
Created attachment 573064 [details] [diff] [review]
fix

This patch switches to use IsResourceDoc() as described above.

It also tweaks the existing reftests img-foreignObject-embed-* as follows:
  - File-rename -- s/embed/iframe/ -- for test w/ iframe, for clarity
  - I modified the SVG helper-file for the test formerly known as img-foreignObject-embed-1.html, such that its <div> is now the *parent* instead of the *sibling* of the <iframe>, so it'll no longer push the <iframe> out of view.  So now, we'd see the <iframe> in the image, if it were being honored.
   - I also split off another version of that test (-1b.html / -1b-helper.svg)  which has no alert() and *just* tests for <iframe> rendering (for the purposes of this next part):
   - I added "!=" lines to ensure that each of the SVG resource files don't match lime.svg when viewed directly.  (though I couldn't do that for the one that uses alert(), since it'd hang the reftest process if viewed directly)
Comment 2 Daniel Holbert [:dholbert] 2011-11-08 19:47:24 PST
Created attachment 573066 [details] [diff] [review]
patch 1 v1

(same patch, just corrected a whitespace issue & alphabetical-ordering in the tweaked reftest.list chunk)
Comment 3 Daniel Holbert [:dholbert] 2011-11-08 19:52:59 PST
Created attachment 573067 [details] [diff] [review]
patch 2 v1  (just a file rename)

This patch just renames img-foreignObject-embed-2* to img-foreignObject-embed-1*, since the old img-foreignObject-embed-1* files are now moved out of the way (since they really use iframe instead of embed).

(I probably could have done this as part of patch 1, but I don't trust hg to do the right thing if I "hg mv XX YY; hg mv ZZ XX" all in the same cset. (and it feels cleaner this way))
Comment 6 Daniel Holbert [:dholbert] 2011-11-12 21:33:55 PST
Created attachment 574118 [details] [diff] [review]
patch 3 v1: s/GetDisplayDocument/IsResourceDoc/ in ShouldLoad

We should make the same sort of change (s/GetDisplayDocument/IsResourceDoc/) in nsDataDocumentContentPolicy::ShouldLoad, too.  Right now ShouldLoad has an early-return for non-external-resource-documents (and then places additional restrictions on external resource documents), and this patch tightens that early-return so that we won't take it for image documents.

This is actually the change that bz originally suggested tweaking back in bug 596765 comment 1.  I hadn't made this change yet, though, because we never actually reach this code for e.g. <iframe> in SVG-as-an-image -- we reject that earlier, in nsFrameLoader::MaybeCreateDocShell (in the chunk of code touched by patch 1 here).  However, if we bypassed that earlier check for some reason, we'd definitely want ShouldLoad to reject it.

This change also will make ShouldLoad clearer -- there's no reason the already-existing check should apply *just* to external resource documents but not to SVG-as-an-image.
Comment 7 Daniel Holbert [:dholbert] 2011-11-12 21:44:14 PST
Created attachment 574119 [details] [diff] [review]
patch 3 v2: s/GetDisplayDocument/IsResourceDoc/ in ShouldLoad

(just fixed a comment after the code-change, to reflect the new behavior)
Comment 8 Daniel Holbert [:dholbert] 2011-11-13 14:25:37 PST
Landed patch 3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff1026f9880
Comment 9 Ed Morley [:emorley] 2011-11-14 04:41:58 PST
https://hg.mozilla.org/mozilla-central/rev/7ff1026f9880

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