99102 – SVGImage::data() returns NULL

Bug 99102 - SVGImage::data() returns NULL

Summary: SVGImage::data() returns NULL

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	SVG (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Philip Rogers

URL:
Keywords:

Depends on:
Blocks:

Reported:	2012-10-11 14:55 PDT by Jay Civelli
Modified:	2012-10-23 20:21 PDT (History)
CC List:	3 users (show)

See Also:

Attachments
First pass (1.83 KB, patch) 2012-10-23 13:45 PDT, Philip Rogers	eric: review+	Details \| Formatted Diff \| Diff
Add back null check per Jay's request (1.84 KB, patch) 2012-10-23 17:10 PDT, Philip Rogers	webkit.review.bot: commit-queue-	Details \| Formatted Diff \| Diff
Updating reviewer (1.84 KB, patch) 2012-10-23 17:30 PDT, Philip Rogers	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

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

Description Jay Civelli 2012-10-11 14:55:17 PDT

Calling SVGImage::data() returns NULL.
It should return the actual SVG data.

Comment 1 Jay Civelli 2012-10-12 10:08:46 PDT

One way to repro is to run the Chromium webkit_unit_tests, more specifically WebPageNewSerializeTest,SVGImageDontCrash.
webkit_unit_tests --gtest_filter=WebPageNewSerializeTest.SVGImageDontCrash

In PageSerializer::addImageToResources() there is a LOG_ERROR that's printed when the SVG image content is missing.

Comment 2 Alexey Proskuryakov 2012-10-12 11:53:07 PDT

Is there any user observable effect?

Comment 3 Jay Civelli 2012-10-12 12:50:53 PDT

The only effect I am aware of is that MHTML generated for a page with SVG images will not include them (as it does not get the data).

Comment 4 Philip Rogers 2012-10-22 09:56:20 PDT

@Jay, were you able to create a repro for this (even if just in c++)?

Comment 5 Jay Civelli 2012-10-22 10:02:50 PDT

@Philip
You can repro with the steps I described in comment #1.
It's a unit-test that generate MHTML from a simple page with an SVG image.

Comment 6 Philip Rogers 2012-10-22 20:51:51 PDT

It looks like the image returned from imageForRenderer() does not contain the copied data() but image->image()->data() does. I should have a patch up tomorrow for this.

Comment 7 Philip Rogers 2012-10-23 13:45:52 PDT

Created attachment 170227 [details]
First pass

Comment 8 Eric Seidel (no email) 2012-10-23 16:16:34 PDT

Comment on attachment 170227 [details]
First pass

LGTM.  Do we know if there is a way to test this with LayoutTests instead of just a Chromium Unit Test?

Comment 9 Jay Civelli 2012-10-23 16:43:28 PDT

LGTM
@eric
We can't test that from a Layout test as page serialization cannot be triggered from a page.

Comment 10 Philip Rogers 2012-10-23 17:10:43 PDT

Created attachment 170271 [details]
Add back null check per Jay's request

Comment 11 WebKit Review Bot 2012-10-23 17:27:55 PDT

Comment on attachment 170271 [details]
Add back null check per Jay's request

Rejecting attachment 170271 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14529170

Comment 12 Philip Rogers 2012-10-23 17:30:00 PDT

Created attachment 170276 [details]
Updating reviewer

Comment 13 WebKit Review Bot 2012-10-23 20:21:23 PDT

Comment on attachment 170276 [details]
Updating reviewer

Clearing flags on attachment: 170276

Committed r132295: <http://trac.webkit.org/changeset/132295>

Comment 14 WebKit Review Bot 2012-10-23 20:21:26 PDT

All reviewed patches have been landed.  Closing bug.