Last Comment Bug 692506 - Regression in SVG content generated via DOMParser.parseFromString() in FF7
: Regression in SVG content generated via DOMParser.parseFromString() in FF7
Status: VERIFIED FIXED
[qa!]
: regression
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla10
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 617623
  Show dependency treegraph
 
Reported: 2011-10-06 10:26 PDT by jeffharris
Modified: 2011-10-25 21:54 PDT (History)
12 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
fixed
+
fixed
fixed


Attachments
svg_image_test.html (808 bytes, text/html)
2011-10-06 10:26 PDT, jeffharris
no flags Details
reference case (466 bytes, text/html)
2011-10-06 12:48 PDT, Daniel Holbert [:dholbert]
no flags Details
Make sure that SVG animated strings keep track of whether the base value is set correctly. (4.16 KB, patch)
2011-10-06 13:01 PDT, Boris Zbarsky [:bz]
dholbert: review+
asa: approval‑mozilla‑aurora+
asa: approval‑mozilla‑beta+
Details | Diff | Review

Description jeffharris 2011-10-06 10:26:56 PDT
Created attachment 565271 [details]
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. 2011-10-06 10:38:39 PDT
See also bug 692089
Comment 2 Boris Zbarsky [:bz] 2011-10-06 10:45:02 PDT
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] 2011-10-06 11:45:47 PDT
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 [:bz] 2011-10-06 11:52:46 PDT
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 [:bz] 2011-10-06 11:58:34 PDT
That used to be a HasAttr() check until bug 617623 (which is in the regression range above) landed.
Comment 6 Boris Zbarsky [:bz] 2011-10-06 12:07:19 PDT
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 [:bz] 2011-10-06 12:14:51 PDT
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] 2011-10-06 12:48:35 PDT
Created attachment 565319 [details]
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 [:bz] 2011-10-06 13:01:09 PDT
Created attachment 565327 [details] [diff] [review]
Make sure that SVG animated strings keep track of whether the base value is set correctly.
Comment 10 Boris Zbarsky [:bz] 2011-10-06 13:03:42 PDT
Daniel, stealing if you don't mind.

Robert, how do you feel about that change for aurora and beta?
Comment 11 Daniel Holbert [:dholbert] 2011-10-06 13:25:31 PDT
(In reply to Boris Zbarsky (:bz) from comment #10)
> Daniel, stealing if you don't mind.
(please do, thanks!)
Comment 12 Daniel Holbert [:dholbert] 2011-10-06 13:48:42 PDT
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] 2011-10-06 13:53:44 PDT
(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 15 Boris Zbarsky [:bz] 2011-10-06 14:15:35 PDT
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 2011-10-06 14:21:55 PDT
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] 2011-10-06 18:08:25 PDT
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 19 Boris Zbarsky [:bz] 2011-10-06 19:45:46 PDT
Jeff, the fix for this should ship in Firefox 8 in about a month...
Comment 20 Ed Morley [:emorley] 2011-10-07 04:11:22 PDT
https://hg.mozilla.org/mozilla-central/rev/baa28d3f6296
Comment 21 Mihaela Velimiroviciu (:mihaelav) 2011-10-07 07:07:28 PDT
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 [:khuey] (khuey@mozilla.com) 2011-10-07 07:09:53 PDT
I think you're using builds from before these fixes landed ...
Comment 23 Boris Zbarsky [:bz] 2011-10-07 07:14:38 PDT
Indeed, that is exactly what Mihaela is doing...
Comment 24 Mihaela Velimiroviciu (:mihaelav) 2011-10-10 02:16:15 PDT
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) 2011-10-14 07:31:20 PDT
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] 2011-10-14 07:50:46 PDT
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) 2011-10-16 23:19:34 PDT
Marking bug as VERIFIED
Comment 28 christian 2011-10-25 21:54:22 PDT
---------------------------------[ 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.

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