Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Check for IsResourceDoc() instead of GetDisplayDocument() in nsFrameLoader::MaybeCreateDocShell(), to more explicitly disable <iframes> in SVG-as-an-image

RESOLVED FIXED in mozilla11

Status

()

Core
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla11
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #573064 - Flags: review?(roc)
(Assignee)

Comment 2

6 years ago
Created attachment 573066 [details] [diff] [review]
patch 1 v1

(same patch, just corrected a whitespace issue & alphabetical-ordering in the tweaked reftest.list chunk)
Attachment #573064 - Attachment is obsolete: true
Attachment #573064 - Flags: review?(roc)
Attachment #573066 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #573066 - Attachment description: fix v1 → patch 1 v1
(Assignee)

Comment 3

6 years ago
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))
Attachment #573067 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Depends on: 596765
Attachment #573066 - Flags: review?(roc) → review+
Attachment #573067 - Flags: review?(roc) → review+
(Assignee)

Comment 4

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6784e7fe0b3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/38586c544d4b
Flags: in-testsuite+
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/6784e7fe0b3b
https://hg.mozilla.org/mozilla-central/rev/38586c544d4b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 6

6 years ago
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.
Attachment #574118 - Flags: review?(roc)
(Assignee)

Comment 7

6 years ago
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)
Attachment #574118 - Attachment is obsolete: true
Attachment #574118 - Flags: review?(roc)
Attachment #574119 - Flags: review?(roc)
Attachment #574119 - Flags: review?(roc) → review+
(Assignee)

Comment 8

6 years ago
Landed patch 3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff1026f9880

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/7ff1026f9880
You need to log in before you can comment on or make changes to this bug.