692506 - Regression in SVG content generated via DOMParser.parseFromString() in FF7

Status: VERIFIED FIXED

(Core :: SVG, defect, P1)

Description

[jeffharris]

Attachment: svg_image_test.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/535.2 (KHTML, like Gecko) Chrome/15.0.874.81 Safari/535.2

Steps to reproduce:

Open the attached svg_image_test.html file


Actual results:

Nothing is displayed


Expected results:

The Google logo should be displayed.

This is a regression. Works fine in FF6, Chrome 15, Safari 5.1, IE9

Comment 1

[j.j.]

See also bug 692089

Comment 2

[Boris Zbarsky [:bzbarsky]]

The behavior changed in this checkin range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=549e3276ed25&tochange=4e3b03de1fd3

The fix for bug 552605 is in that range.  Joe, could that be causing problems here?

Comment 3

[Daniel Holbert [:dholbert]]

AFAICT no images are involved here, just raw SVG content generated via DOMParser() (which I'm not familiar with).

Shifting to Core:SVG, and changing branch to "Trunk" as I can reproduce (testcase is blank) in today's nightly:
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111005 Firefox/10.0a1

Comment 4

[Boris Zbarsky [:bzbarsky]]

When the <svg:image> is bound to the tree after that last append,  (mStringAttributes[HREF].IsExplicitlySet()) tests false so the load start from BindToTree is skipped.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That used to be a HasAttr() check until bug 617623 (which is in the regression range above) landed.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So what we do right now is explicitly kick off an image load from nsSVGImageElement::AfterSetAttr.  This, of course, does nothing in the data document.

Robert, should we be changing mStringAttributes[HREF] somewhere here?

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, so we're coming from nsSVGElement::ParseAttribute and we call nsSVGString::SetBaseValue with aDoSetAttr == false... which never sets mBaseIsSet.  That seems wrong.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: reference case

(Here's a reference case -- it's the same as the testcase, but with the SVG directly pasted into the html, rather than being generated/inserted via script with DOMParser().  This version renders correctly on my machine.)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure that SVG animated strings keep track of whether the base value is set correctly.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Daniel, stealing if you don't mind.

Robert, how do you feel about that change for aurora and beta?

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #10)
> Daniel, stealing if you don't mind.
(please do, thanks!)

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 565327 [details] [diff] [review]
Make sure that SVG animated strings keep track of whether the base value is set correctly.

Patch looks correct to me, FWIW -- and I reviewed the original cset that regressed this (sorry for not catching it there!) -- hence, stealing review, so this can get fixed faster.

In case it's helpful to anyone else, here's what I just re-verified myself: we skip the SetStringBaseValue call in this case because SetStringBaseValue is effectively a wrapper for nsGenericElement::SetAttr -- and in this case, we're already *inside* of SetAttr.  (nsGenericElement::SetAttr calls nsSVGElement::ParseAttribute which calls nsSVGString::SetBaseValue)

The actual attribute-setting takes place at the end of nsSVGElement::ParseAttribute -- if (foundMatch) { [..] aResult.SetTo(aValue);

So anyway -- we skip the SetStringBaseValue, since we know the attribute-setting is already being handled at a higher level, but we do absolutely still need to keep track of the fact that the value's been set. So, mIsBaseSet does need to be set unconditionally there.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #10)
> Robert, how do you feel about that change for aurora and beta?

(I'll comment on this as well since I stole review :)  This isn't zero-risk, but I think it's targeted enough to be safe for aurora.  I would want to audit the callers of nsSVGString::IsExplicitlySet a bit more before rendering an opinion about suitability for beta.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/baa28d3f6296

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 565327 [details] [diff] [review]
Make sure that SVG animated strings keep track of whether the base value is set correctly.

OK.  I'm going to request approval for both aurora and beta for the moment; I too think this is a clear slam-dunk for aurora and that someone who knows this code better than I should evaluate risk for beta...

Comment 16

[Robert Longson [:longsonr]]

The fix is clearly right. No other mIsBaseSet in any other type is dependent on aDoSetAttr so this makes us match integers, enums etc. Looks pretty good value for a 1 line change to me.

Comment 17

[Daniel Holbert [:dholbert]]

Yup -- after re-skimming relevant parts of the patch that regressed this, I'm definitely comfortable with taking this on beta.

The regressing patch (for bug 617623) replaced some nsSVGElement::HasAttr([string_attribute_name]) calls with calls to nsSVGString::IsExplicitlySet().  (I believe that covers all of the nsSVGString::IsExplicitlySet() calls that exist.)

In order for us to preserve old/correct behavior & continue returning true in all cases when HasAttr() would have originally returned true, we definitely need this bug's patch.

Comment 18

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4216dc9744ed
https://hg.mozilla.org/releases/mozilla-beta/rev/570515d49188

Comment 19

[Boris Zbarsky [:bzbarsky]]

Jeff, the fix for this should ship in Firefox 8 in about a month...

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/baa28d3f6296

Comment 21

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111006 Firefox/9.0a2
Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111006 Firefox/10.0a1

Opening the svg_image_test.html file from bug description displays a blank page.
Opening the reference case from comment #8 diplays the Google logo, but also a security warning pup-up ("You have requested an encrypted page that contains some unencrypted information. Information that you see or enter on this page could easily be read by a third party.").

In Chrome, both pages display the Google logo with no security warning.

Is this the expected behaviour? or can this bug be reopened?

Thank you!

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think you're using builds from before these fixes landed ...

Comment 23

[Boris Zbarsky [:bzbarsky]]

Indeed, that is exactly what Mihaela is doing...

Comment 24

[Mihaela Velimiroviciu (:mihaelav)]

Verified as fixed - google logo is displayed when opening the test pages - on the builds bellow(latest Aurora and Nightly):
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111009 Firefox/9.0a2
Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111009 Firefox/10.0a1
Mozilla/5.0 (Windows NT 5.1; rv:9.0a2) Gecko/20111009 Firefox/9.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a2) Gecko/20111009 Firefox/9.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a1) Gecko/20111009 Firefox/10.0a1
Mozilla/5.0 (X11; Linux i686; rv:10.0a1) Gecko/20111009 Firefox/10.0a1

Besides the google logo, a security warning was displayed when opening svg_image_test.html on:
Mozilla/5.0 (Windows NT 5.1; rv:10.0a1) Gecko/20111009 Firefox/10.0a1
Mozilla/5.0 (X11; Linux i686; rv:9.0a2) Gecko/20111009 Firefox/9.0a2

Need to verify on Beta (8.0) when a new build will be available.

Comment 25

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (X11; Linux i686; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0

Verified the fix on Firefox 8 beta 3: the Google logo is displayed.

Also, a security warning (the same as in comment #21 and comment #24) was displayed on Mac and Win7 when opening svg_image_test.html.
Is this expected?

Comment 26

[Daniel Holbert [:dholbert]]

Yes, the security warning is expected. (The testcase is loaded over HTTPS, but its Google logo image is referenced using unsecure HTTP, so that warning is to let you know that a man-in-the-middle could be messing with your Google logo.)

Thanks for the verification!

Comment 27

[Mihaela Velimiroviciu (:mihaelav)]

Marking bug as VERIFIED

Comment 28

[LegNeato]

---------------------------------[ Triage Comment ]---------------------------------

Marking this as tracking as this was a shipped regression that Google wants us to fix (though they have put in a workaround).

The fix has already landed but it doesn't hurt to track to make sure it isn't backed out, etc. We have confirmed that fixing this won't break their workaround as well.