Bug 46120 – RenderImage::intrinsicSizeChanged crashes when m_imageResource is missing

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Cosmin Truta]

This is a regression from bug 43779, changeset 66223. It has been noticed in Chromium; see the Chromium issue 54972 at
http://code.google.com/p/chromium/issues/detail?id=54972

I am submitting a straightforward patch that fixes the crash, although I am not entirely sure this is the right fix. Input from reviewers will be highly appreciated.

My understanding of the changeset 66223 is that RenderImage objects must have an ImageResource, so setImageResource must be called either inside the constructor (as it's the case with RenderMedia), or immediately after the constructor (as it's the case with the renderer associated with HTMLImageElement, HTMLInputElement, HTMLObjectElement, etc.). This, however, doesn't work well in the case of RenderObject or RenderObjectChildList: inside RenderObject::createObject, as well as inside RenderObjectChildList::updateBeforeAfterContent, setImageResource is called only after setting the image style.

This leads to a crash when the style is zoomed, because RenderImage::setStyle (or more precisely, RenderImage::intrinsicSizeChanged) assumes that an ImageResource object exists. Swapping setStyle and setImageResource doesn't work any better, because setImageResource also assumes that a style exists!

I am setting the severity/priority level to Major/P1, because it is crashing Chromium badly.
I will submit, shortly, my very simple patch and layout test.

[Cosmin Truta]

Created attachment 68179 [details]
Fix and layout test

As I mentioned in the previous comment, perhaps a little ambiguously:
Swapping the order of calls to setStyle/setImageResource inside RenderObject::createObject and RenderObjectChildList::updateBeforeAfterContent does not solve the lack of initialization, because initializing a style and initializing an image resource are cyclically dependent on each other. I am, therefore, relying on a simple if-check instead, to avoid this issue altogether.

[Nikolas Zimmermann]

Comment on attachment 68179 [details]
Fix and layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=68179&action=review

> WebCore/rendering/RenderImage.h:66
> +        if (m_imageResource.get())

No need to use .get().

r=me. If you don't have commit access, you'll need to upload a new version, that I can cq+.

[Cosmin Truta]

Created attachment 68226 [details]
Fix and layout test, comply with reviewer's comments

[WebKit Commit Bot]

Comment on attachment 68226 [details]
Fix and layout test, comply with reviewer's comments

Rejecting patch 68226 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Projects/CommitQueue/LayoutTests
Testing 21333 test cases.
css3/style-zoomed-image.html -> failed

Exiting early after 1 failures. 1581 tests run.
48.40s total testing time

1580 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

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

[Cosmin Truta]

(In reply to comment #4)
Did the layout test fail on Mac? I do not understand why.
I am running the trybot, again, to get more details on why it happened. I do not know, at this point, whether it's my fault and it needs more fixing on Mac. The patch is urgently needed on Chromium; should I add an exclusion, for the time being?

[James Robinson]

The commit queue is saying that this test failed on mac.  I'll see if I can repro.

[Cosmin Truta]

I rerun my patch with the try bots, thinking that I might have done something wrong previously, but I received successful results this time as well. I am really confused by what is happening, and I would really appreciate some help. Is there a way to retry this, e.g. by putting the patch back into the commit queue?

[James Robinson]

(In reply to comment #7)
> I rerun my patch with the try bots, thinking that I might have done something wrong previously, but I received successful results this time as well. I am really confused by what is happening, and I would really appreciate some help. Is there a way to retry this, e.g. by putting the patch back into the commit queue?

What platform are you testing on?  The failure the commit queue is seeing is on the mac port (i.e. Safari), not chromium-mac.

[James Robinson]

The failure is due to trailing whitespace differences.  I'm guessing you were testing with new-run-webkit-tests, which ignores trailing '\n's, I can fix and land by hand if you'd like.

[Cosmin Truta]

Created attachment 68320 [details]
Fix + layout test + fixed "expected" file

James, I'm so grateful for finding this out for me. Thank you!

I don't know how I lost the trailing '\n', probably while copying+pasting, and their lack didn't seem to cause any issue in the Chromium test runs.
When I resubmitted the job to the trybots, all of them (mac, linux, windows) were happy as well.

I already resubmitting the fixed patch, could you please review and commit-queue it?
(Or, should I have submitted only the '\n\n' change this time?)

[James Robinson]

Comment on attachment 68320 [details]
Fix + layout test + fixed "expected" file

As I said, new-run-webkit-tests (which chromium uses) ignores trailing newline diffs when running tests.  run-webkit-tests (which the mac port uses) does not.  That's why you did not see a failure but the commit queue did.

[James Robinson]

Comment on attachment 68320 [details]
Fix + layout test + fixed "expected" file

Clearing flags on attachment: 68320

Committed r68004: <http://trac.webkit.org/changeset/68004>

[James Robinson]

All reviewed patches have been landed.  Closing bug.