Make Image::image_observer_ WeakPersistent

Make Image::image_observer_ WeakPersistent

|image_observer_| was made UntracedMember during Oilpanization in [1],
and this CL turns it WeakPersistent, to clear |image_observer_|
when the corresponding ImageResourceContent is garbage collected while
Image is still alive because the Image is referenced from other than
ImageResourceContent.

This CL also add null checks of GetImageObserver() to SVG-related code,
because GetImageObserver() can be null since before.

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

BUG=726220
Review-Url: https://codereview.chromium.org/2905833003
Cr-Commit-Position: refs/heads/master@{#475116}
Committed: https://chromium.googlesource.com/chromium/src/+/e056249f434f9ccca8024246ebb879d225b9f55a

[hiroshige, 2017-05-25 21:49:54]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 21:50:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-25 21:54:55]

Description was changed from

==========
Clear Image::image_observer_ when ImageResourceContent is garbage collected

BUG=726220
==========

to

==========
Make Image::image_observer_ WeakPersistent

|image_observer_| was made UntracedMember during Oilpanization in [1],
and this CL turns it WeakPersistent, to clear |image_observer_|
when the corresponding ImageResourceContent is garbage collected while
Image is still alive because the Image is referenced from other than
ImageResourceContent.

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

BUG=726220
==========

[hiroshige, 2017-05-25 21:58:37]

hiroshige@chromium.org changed reviewers:
+ hajimehoshi@chromium.org, haraken@chromium.org, kouhei@chromium.org,
sigbjornf@opera.com

[hiroshige, 2017-05-25 22:00:50]

hiroshige@chromium.org changed reviewers:
+ fs@opera.com

[hiroshige, 2017-05-25 22:03:33]

hiroshige@chromium.org changed reviewers:
+ kinuko@chromium.org

[hiroshige, 2017-05-25 22:03:34]

PTAL,

sof@ and haraken@, as the author and reviewer of
https://codereview.chromium.org/1610883002 that made image_observer_
UntracedMember instead of WeakPersistent.

kouhei@, kinuko@, fs@
for SVG and as the reviewers of my regressing CL
https://codereview.chromium.org/2897533002.

hajimehoshi@ for Image lifetime.

[hiroshige, 2017-05-25 22:06:50]

Description was changed from

==========
Make Image::image_observer_ WeakPersistent

|image_observer_| was made UntracedMember during Oilpanization in [1],
and this CL turns it WeakPersistent, to clear |image_observer_|
when the corresponding ImageResourceContent is garbage collected while
Image is still alive because the Image is referenced from other than
ImageResourceContent.

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

BUG=726220
==========

to

==========
Make Image::image_observer_ WeakPersistent

|image_observer_| was made UntracedMember during Oilpanization in [1],
and this CL turns it WeakPersistent, to clear |image_observer_|
when the corresponding ImageResourceContent is garbage collected while
Image is still alive because the Image is referenced from other than
ImageResourceContent.

This CL also add null checks of GetImageObserver() to SVG-related code,
because GetImageObserver() can be null since before.

that are necessary
for make the added layout test to pass, because


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

BUG=726220
==========

[hiroshige, 2017-05-25 22:17:30]

Description was changed from

==========
Make Image::image_observer_ WeakPersistent

|image_observer_| was made UntracedMember during Oilpanization in [1],
and this CL turns it WeakPersistent, to clear |image_observer_|
when the corresponding ImageResourceContent is garbage collected while
Image is still alive because the Image is referenced from other than
ImageResourceContent.

This CL also add null checks of GetImageObserver() to SVG-related code,
because GetImageObserver() can be null since before.

that are necessary
for make the added layout test to pass, because


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

BUG=726220
==========

to

==========
Make Image::image_observer_ WeakPersistent

|image_observer_| was made UntracedMember during Oilpanization in [1],
and this CL turns it WeakPersistent, to clear |image_observer_|
when the corresponding ImageResourceContent is garbage collected while
Image is still alive because the Image is referenced from other than
ImageResourceContent.

This CL also add null checks of GetImageObserver() to SVG-related code,
because GetImageObserver() can be null since before.

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

BUG=726220
==========

[haraken, 2017-05-25 23:23:02]

LGTM

https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
(right):

https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:120: //
this explicit lifetime check.

Is it still hard to move Image to Oilpan's heap?

https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:122:
ThreadHeap::WillObjectBeLazilySwept(image_->GetImageObserver()))

Now that Image::image_observer_ is cleared in the stop-the-world phase, we can
remove the WillObjectBeLazilySwept check.

+Sigbjorn for double-checking.

[hiroshige, 2017-05-26 00:03:27]

https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
(right):

https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:120: //
this explicit lifetime check.
On 2017/05/25 23:23:02, haraken wrote:
> 
> Is it still hard to move Image to Oilpan's heap?

I'm not sure. Were there any blocking issues to move Image to Oilpan heap?

(Related issue: Issue 719617 about thread affinity of Image)

[commit-bot: I haz the power, 2017-05-26 02:16:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 02:16:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kinuko, 2017-05-26 04:44:19]

I'm probably not the best reviewer in this area, while the change itself looks
pretty reasonable to me.

[haraken, 2017-05-26 04:48:01]

On 2017/05/26 00:03:27, hiroshige wrote:
>
https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
> (right):
> 
>
https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:120: //
> this explicit lifetime check.
> On 2017/05/25 23:23:02, haraken wrote:
> > 
> > Is it still hard to move Image to Oilpan's heap?
> 
> I'm not sure. Were there any blocking issues to move Image to Oilpan heap?
> 
> (Related issue: Issue 719617 about thread affinity of Image)

Either way this CL LGTM % removing the WillObjectBeLazilySwept check.

[fs, 2017-05-26 08:28:36]

https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/svg/as-image/adopt-while-async-svg-load-is-in-progress-crash.html
(right):

https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/as-image/adopt-while-async-svg-load-is-in-progress-crash.html:30:
setTimeout(t.step_func_done(function() {}), 100);
Nit: Don't need an empty function here. (Can just do step_func_done().)

[kouhei (in TOK), 2017-05-26 11:17:17]

lgtm % fs comment

[hiroshige, 2017-05-26 18:44:02]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 18:44:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2017-05-26 18:44:58]

https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/svg/as-image/adopt-while-async-svg-load-is-in-progress-crash.html
(right):

https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/as-image/adopt-while-async-svg-load-is-in-progress-crash.html:30:
setTimeout(t.step_func_done(function() {}), 100);
On 2017/05/26 08:28:36, fs wrote:
> Nit: Don't need an empty function here. (Can just do step_func_done().)

Done.

https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp
(right):

https://codereview.chromium.org/2905833003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/graphics/SVGImageChromeClient.cpp:122:
ThreadHeap::WillObjectBeLazilySwept(image_->GetImageObserver()))
On 2017/05/25 23:23:02, haraken wrote:
> 
> Now that Image::image_observer_ is cleared in the stop-the-world phase, we can
> remove the WillObjectBeLazilySwept check.
> 
> +Sigbjorn for double-checking.

Done.

[hiroshige, 2017-05-26 18:46:16]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2017-05-26 18:46:24]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-05-26 18:46:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2905833003/#ps20001
(title: "Reflect comments")

[commit-bot: I haz the power, 2017-05-26 18:46:47]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 20:43:42]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1495824384193690,
"parent_rev": "4b97690be1738233d04011eee439dd58b17eb05b", "commit_rev":
"e056249f434f9ccca8024246ebb879d225b9f55a"}

[commit-bot: I haz the power, 2017-05-26 20:43:57]

Description was changed from

==========
Make Image::image_observer_ WeakPersistent

|image_observer_| was made UntracedMember during Oilpanization in [1],
and this CL turns it WeakPersistent, to clear |image_observer_|
when the corresponding ImageResourceContent is garbage collected while
Image is still alive because the Image is referenced from other than
ImageResourceContent.

This CL also add null checks of GetImageObserver() to SVG-related code,
because GetImageObserver() can be null since before.

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

BUG=726220
==========

to

==========
Make Image::image_observer_ WeakPersistent

|image_observer_| was made UntracedMember during Oilpanization in [1],
and this CL turns it WeakPersistent, to clear |image_observer_|
when the corresponding ImageResourceContent is garbage collected while
Image is still alive because the Image is referenced from other than
ImageResourceContent.

This CL also add null checks of GetImageObserver() to SVG-related code,
because GetImageObserver() can be null since before.

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

BUG=726220

Review-Url: https://codereview.chromium.org/2905833003
Cr-Commit-Position: refs/heads/master@{#475116}
Committed:
https://chromium.googlesource.com/chromium/src/+/e056249f434f9ccca8024246ebb8...
==========

[commit-bot: I haz the power, 2017-05-26 20:43:58]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e056249f434f9ccca8024246ebb8...