Bug 46120 - RenderImage::intrinsicSizeChanged crashes when m_imageResource is missing
Summary: RenderImage::intrinsicSizeChanged crashes when m_imageResource is missing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-20 14:24 PDT by Cosmin Truta
Modified: 2010-09-21 18:09 PDT (History)
2 users (show)

See Also:


Attachments
Fix and layout test (3.99 KB, patch)
2010-09-20 19:53 PDT, Cosmin Truta
zimmermann: review+
zimmermann: commit-queue-
Details | Formatted Diff | Diff
Fix and layout test, comply with reviewer's comments (3.98 KB, patch)
2010-09-21 05:54 PDT, Cosmin Truta
simon.fraser: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Fix + layout test + fixed "expected" file (3.99 KB, patch)
2010-09-21 17:54 PDT, Cosmin Truta
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cosmin Truta 2010-09-20 14:24:03 PDT
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.
Comment 1 Cosmin Truta 2010-09-20 19:53:52 PDT
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.
Comment 2 Nikolas Zimmermann 2010-09-20 22:51:43 PDT
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+.
Comment 3 Cosmin Truta 2010-09-21 05:54:24 PDT
Created attachment 68226 [details]
Fix and layout test, comply with reviewer's comments
Comment 4 WebKit Commit Bot 2010-09-21 14:36:07 PDT
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
Comment 5 Cosmin Truta 2010-09-21 14:56:36 PDT
(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?
Comment 6 James Robinson 2010-09-21 16:57:25 PDT
The commit queue is saying that this test failed on mac.  I'll see if I can repro.
Comment 7 Cosmin Truta 2010-09-21 17:01:49 PDT
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?
Comment 8 James Robinson 2010-09-21 17:11:47 PDT
(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.
Comment 9 James Robinson 2010-09-21 17:20:08 PDT
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.
Comment 10 Cosmin Truta 2010-09-21 17:54:48 PDT
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?)
Comment 11 James Robinson 2010-09-21 17:58:31 PDT
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.
Comment 12 James Robinson 2010-09-21 18:08:58 PDT
Comment on attachment 68320 [details]
Fix + layout test + fixed "expected" file

Clearing flags on attachment: 68320

Committed r68004: <http://trac.webkit.org/changeset/68004>
Comment 13 James Robinson 2010-09-21 18:09:14 PDT
All reviewed patches have been landed.  Closing bug.