File attached

Deleted: test.html 1006 bytes

test.html 1006 bytes View Download

Also applies to Mac OS 10.12.3, Chrome 56.0.2924.87

Thanks for the reproduction case!  It is really appreciated.

Reproduces on Linux as well, so assuming this bug is neutral to all
platforms.

On debug build, it stepped on an assertion.
Can anybody in canvas/SVG team take a look?

[1:1:0210/144434.290668:FATAL:SVGImage.cpp(98)] Check failed: frame->document()->loadEventFinished(). 
#0 0x7f240d1ebb1b base::debug::StackTrace::StackTrace()
#1 0x7f240d1ea15c base::debug::StackTrace::StackTrace()
#2 0x7f240d25518f logging::LogMessage::~LogMessage()
#3 0x7f23f4170a0f blink::SVGImage::currentFrameHasSingleSecurityOrigin()
#4 0x7f23f3ee86b0 blink::ImageResourceContent::isAccessAllowed()
#5 0x7f23f37aef2e blink::HTMLImageElement::wouldTaintOrigin()
#6 0x7f23f3874a58 blink::CanvasRenderingContext::wouldTaintOrigin()
#7 0x7f23f19d1aeb blink::CanvasRenderingContext2D::wouldTaintOrigin()
#8 0x7f23f19bb926 blink::BaseRenderingContext2D::drawImage()
#9 0x7f23f19b9e7f blink::BaseRenderingContext2D::drawImage()
#10 0x7f23f14b12f4 blink::CanvasRenderingContext2DV8Internal::drawImage1Method()
#11 0x7f23f14a3b14 blink::CanvasRenderingContext2DV8Internal::drawImageMethod()
#12 0x7f23f14a3a75 blink::V8CanvasRenderingContext2D::drawImageMethodCallback()
#13 0x7f23fde9777b v8::internal::FunctionCallbackArguments::Call()
#14 0x7f23fdf67ca3 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#15 0x7f23fdf667e0 v8::internal::Builtin_Impl_HandleApiCall()
#16 0x1d281050420e <unknown>

Received signal 6
#0 0x7f240d1ebb1b base::debug::StackTrace::StackTrace()
#1 0x7f240d1ea15c base::debug::StackTrace::StackTrace()
#2 0x7f240d1eb62f base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7f240d652330 <unknown>
#4 0x7f23f9537c37 gsignal
#5 0x7f23f953b028 abort
#6 0x7f240d1e7ed6 base::debug::(anonymous namespace)::DebugBreak()
#7 0x7f240d1e7eb8 base::debug::BreakDebugger()
#8 0x7f240d255526 logging::LogMessage::~LogMessage()
#9 0x7f23f4170a0f blink::SVGImage::currentFrameHasSingleSecurityOrigin()
#10 0x7f23f3ee86b0 blink::ImageResourceContent::isAccessAllowed()
#11 0x7f23f37aef2e blink::HTMLImageElement::wouldTaintOrigin()
#12 0x7f23f3874a58 blink::CanvasRenderingContext::wouldTaintOrigin()
#13 0x7f23f19d1aeb blink::CanvasRenderingContext2D::wouldTaintOrigin()
#14 0x7f23f19bb926 blink::BaseRenderingContext2D::drawImage()
#15 0x7f23f19b9e7f blink::BaseRenderingContext2D::drawImage()
#16 0x7f23f14b12f4 blink::CanvasRenderingContext2DV8Internal::drawImage1Method()
#17 0x7f23f14a3b14 blink::CanvasRenderingContext2DV8Internal::drawImageMethod()
#18 0x7f23f14a3a75 blink::V8CanvasRenderingContext2D::drawImageMethodCallback()
#19 0x7f23fde9777b v8::internal::FunctionCallbackArguments::Call()
#20 0x7f23fdf67ca3 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#21 0x7f23fdf667e0 v8::internal::Builtin_Impl_HandleApiCall()
#22 0x1d281050420e <unknown>
  r8: 00007ffe583784e0  r9: 00007f23f9651a00 r10: 0000000000000008 r11: 0000000000000202
 r12: 0000000000000000 r13: 00007ffe58379e78 r14: 00000292d2860020 r15: 00007f23f14a3a60
  di: 0000000000000001  si: 0000000000000001  bp: 00007ffe583788d0  bx: 00007ffe58379df0
  dx: 0000000000000006  ax: 0000000000000000  cx: ffffffffffffffff  sp: 00007ffe58378798
  ip: 00007f23f9537c37 efl: 0000000000000202 cgf: 0000000000000033 erf: 0000000000000000
 trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]

Using the per-revision bisect providing the bisect results,

You are probably looking for a change made after 430571 (known good), but no later than 430572 (first known bad).

CHANGE-LOG URL:
---------------------------------------
https://chromium.googlesource.com/chromium/src/+log/5db5553160affb9ea2464666320c3855e5d203d2..b8cb16f79a95c8bc639a9c58785a8484c197aca9

From the CL above, assigning the issue to the concern owner
Review-Url: https://codereview.chromium.org/2269043002

hiroshige@ - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Thanks!

This is likely related to the disconnect between "image (data) loaded" and the 'load' event actual firing. We've seen this before for instance with fonts (issue 382170). It does look likely that the CL above would have made this show up.

> #5
Agree.

I'm working again on Isue 382170.

Progress?

Still working, mainly on the design issue: who should block Document load event?
I'll put a doc to Issue 382170 later.

Draft CL: https://codereview.chromium.org/2613853002/

Any action on this? The code review generally seems to support moving forward, although with some changes.

We're trying to fix all P1 bugs within 30 days. Maybe this is not a P1, given the use case probably doesn't come up too often.

I'm blocked by some concerns around https://codereview.chromium.org/2746343002/ (and also was busy for another issue that I've just finished). I'd like to restart the effort and revise the design soon.

I'm marking this as a beta release blocker, because in theory all P1s should get fixed inside of 30 days. We can always punt if necessary, given the challenges of this bug.

Restarted.

Friendly ping to get an update on this issue marked as Beta blocker.

I don't intend to block M-59 beta.
The root cause is Issue 382170 that have been open for a long time.
I'm targetting M-60 and draft CLs are under review, but I have no plan to merge them to M-59.

c14 sgtm

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ebd2fa73e3046f3d6397545285a41fa1d78c4be8

commit ebd2fa73e3046f3d6397545285a41fa1d78c4be8
Author: hiroshige <hiroshige@chromium.org>
Date: Mon May 08 20:23:29 2017

Make ImageResourceContent manage its own ResourceStatus

Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit#

1. This CL
- Adds ImageResourceContent::content_status_,
- Renames ImageResourceContent::GetStatus() to GetContentStatus(), and
- Makes them updated independently from Resource::status_/GetStatus().

This is to decouple GetStatus() timing dependencies
between ImageResource and ImageResourceContent, particularly to allow
ImageResourceContent::GetContentStatus() to be changed to kCached
(in https://codereview.chromium.org/2613853002/) when SVG image loading
is really finished, not necessarily when ResourceLoader is finished.

2. In order to change the content state to kPending when loading is
   started, NotifyStartLoad() notifications are introduced.

3. Stricter assertions are added to |content_status_| transitions.
   |SetStatus(kPending)| and NotifyStartLoad() calls are added to
   non-ResourceLoader paths to make the state transition consistent.

4. This CL makes ImageResourceContent::GetContentStatus() to be kCached
   before ImageNotifyFinished() is called in successful loading,
   while Resource::GetStatus() is kPending at that time.

BUG=667641, 382170,  690480 

Review-Url: https://codereview.chromium.org/2746343002
Cr-Commit-Position: refs/heads/master@{#470104}

[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/css/cssom/CSSStyleImageValue.h
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/html/HTMLObjectElement.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/ImageResource.h
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/ImageResourceInfo.h
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/core/loader/resource/MockImageResourceObserver.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/platform/loader/fetch/Resource.h
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/ebd2fa73e3046f3d6397545285a41fa1d78c4be8/third_party/WebKit/Source/web/tests/WebFrameTest.cpp

ajha@, note the milestone for the beta block is M-60. That is, the intention is to fix it before M-60 is promoted to beta.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29c12a26e644a95e6d53eb7e02c136ca9fdf4c94

commit 29c12a26e644a95e6d53eb7e02c136ca9fdf4c94
Author: hiroshige <hiroshige@chromium.org>
Date: Thu May 11 23:10:57 2017

Fix ImageResourceContent::Create()'s GetContentStatus()

Follow-up of a regression bug caused by [1]:
Previously, ImageResourceContent::Create() returned a content with
kCached, while after [1] it returns a content with kNotStarted.
This causes behavior changes if Create() is called with non-null
argument.

[1] https://codereview.chromium.org/2746343002/

This CL fixes this issue by renaming Create() with a non-null argument
to ImageResourceContent::CreateLoaded() that sets the status to kCached.
The callsites of CreateLoaded() were affected by the bug, i.e.
a callsite in HTMLImageElement and others in tests.

Create() with the nullptr argument is renamed to CreateNotStarted(),
which is only called from ImageResource and is not affected by the bug.

BUG=667641, 382170,  690480 

Review-Url: https://codereview.chromium.org/2869733004
Cr-Commit-Position: refs/heads/master@{#471113}

[modify] https://crrev.com/29c12a26e644a95e6d53eb7e02c136ca9fdf4c94/third_party/WebKit/Source/core/frame/ImageBitmapTest.cpp
[modify] https://crrev.com/29c12a26e644a95e6d53eb7e02c136ca9fdf4c94/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/29c12a26e644a95e6d53eb7e02c136ca9fdf4c94/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/29c12a26e644a95e6d53eb7e02c136ca9fdf4c94/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/29c12a26e644a95e6d53eb7e02c136ca9fdf4c94/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/396a1266fb7549e03e5bcff793ac138654e52d3a

commit 396a1266fb7549e03e5bcff793ac138654e52d3a
Author: hiroshige <hiroshige@chromium.org>
Date: Tue May 16 22:17:35 2017

Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes

design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit#
https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8voc/edit#

This CL calls ImageResourceObserver::ImageNotifyFinished()
after SVG image loading is completed, not necessarily the same time as
ResourceLoader completion (e.g. DidFinishLoading()).
This makes SVG's <img> load event to be fired after SVG image loading
is completed even when the SVG has subresources loaded asynchronously.

1. Image::SetData() can return SizeAvailableAndLoadingAsynchronously
   when SVG image loading is not completed synchronously (due to
   subresources), and notifies the completion asynchronously via
   ImageObserver:LoadCompleted().
   LocalFrameClient::DispatchDidHandleOnloadEvents() is used as a
   notification of SVG image loading completion in SVGImage.

2. ImageResourceContent delays calling ImageNotifyFinished() and
   setting |content_status_| to kCached until SVG image loading is
   completed.

3. ImageLoader additionally delays the Document load event
   - from the first ImageChanged() during image loading
   - until (potentially delayed) ImageNotifyFinished().

In image loading not by ImageLoader, the Document load event is not
delayed from ResourceLoader completion until ImageNotifyFinished(),
but this is not a regression.

In image loading by ImageLoader, the Document load event is delayed
until ImageNotifyFinished() by the combination of
ResourceFetcher::loaders_ and ImageLoader.
ImageLoader starts delaying the Document load event
- Not from |image_| is set, because we shouldn't delay the Document
  load event when image loading is deferred.
- Not from ImageResourceContent::NotifyStartLoad(), to avoid additional
  ImageResourceObserver callbacks.

BUG=382170,  690480 , 667641

Review-Url: https://codereview.chromium.org/2613853002
Cr-Commit-Position: refs/heads/master@{#472229}

[add] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html
[add] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-data-url-crash.html
[add] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-font-crash.html
[add] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/LayoutTests/svg/as-image/resources/data-font-in-css-invalid-data-url.svg
[add] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/LayoutTests/svg/as-image/resources/data-font-in-css-invalid-font.svg
[add] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/LayoutTests/svg/as-image/resources/data-font-in-css.svg
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/ImageLoader.h
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/resource/ImageResource.h
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/svg/graphics/SVGImage.h
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/core/svg/graphics/SVGImageTest.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/platform/graphics/Image.h
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/platform/graphics/ImageObserver.h
[modify] https://crrev.com/396a1266fb7549e03e5bcff793ac138654e52d3a/third_party/WebKit/Source/platform/loader/fetch/Resource.h

The fix (Comment #19) was landed on 60.0.3102.0.

Just to update, no crash is seen on the latest M-60(60.0.3107.0) on Windows-10, Mac OS 10.12.4 and Linux Ubuntu 14.04 as per the attached test file(test.html) in C#1. Hence adding the verified label.

hiroshige@: Could you please close the issue if there is no further work to be done here.


Note: There are very few crash instances seen with this magic signature on M-59. Link to the builds/OS: https://goto.google.com/ouhov. 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8952091b786ce79e9e10810d2b047d39c420d4d7

commit 8952091b786ce79e9e10810d2b047d39c420d4d7
Author: hiroshige <hiroshige@chromium.org>
Date: Mon May 22 16:24:08 2017

Make ImageObserver::AsyncLoadCompleted() called asynchronously

- To avoid complex things (i.e. ImageNotifyFinished() calls)
  during Document::ImplicitClose() to prevent timing bugs, and
- To make LoadEventFinished() of SVG's document true when
  ImageNotifyFinished() is called, and make the CHECK() in
  https://codereview.chromium.org/2888953004/ hold.

BUG=382170,  690480 , 667641

Review-Url: https://codereview.chromium.org/2897533002
Cr-Commit-Position: refs/heads/master@{#473593}

[modify] https://crrev.com/8952091b786ce79e9e10810d2b047d39c420d4d7/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
[modify] https://crrev.com/8952091b786ce79e9e10810d2b047d39c420d4d7/third_party/WebKit/Source/core/svg/graphics/SVGImage.h

Thanks for verification! > #21

All planned CLs have been landed. Closing.

Note: This is not mergeable to M-59 because it contains substantial changes to image loading.

No crash is observed on latest M60 - 60.0.3107.3 as per the steps mentioned in the comment#0. Tested on Samsung galaxy Tab S2 (SM-T815Y) / MMB29K , Nexus 6P/N , Asus Zenfone 2 (X86_Z00AD) / LRX21V. Thanks