Based on  issue 656494 , c-ing few devs. could domeone look into this issue?
Thank you.

Predator and CL did not provide any possible suspects.
Using Code Search for the file, "ElementVisibilityObserver.cpp" assigning to the concern owner.
Suspecting Commit#
https://chromium.googlesource.com/chromium/src/+/10fecf4ab18ee7f481d25ae31122bb099c9b7039

@mlamouri -- Could you please look into the issue, kindly re-assign if this is not related to your changes.
Thank You.

I'll take a initial look as mlamouri@ is at BlinkOn

I'm seeing HTMLMediaElement::onVisibilityChangedForAutoplay() is called after we stop the ElementVisibilityObserver(), i.e. visibility change can still be fired after IntersectionObserver::Stop().

szager@, is this possible?

A quick fix is to just add a null check in HTMLMediaElement::onVisibilityChangedForAutoplay()

That should not be possible, I'd be very curious to see how that could happen.

It is actually possible. Found this: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp?rcl=bbf5af70f469a5f7807ec4573a65c3710cfbb29a&l=35

IntersectionObserverController::postTaskToDeliverObservations() posts a delayed task, which eventually reaches HTMLMediaElement::onVisibilityChangedForAutoplay().

If IntersectionObserver::stop() is called before the delayed task to execute, then we can reach this crash.

Do you think I should go with the quick fix in #6?

Ah, I see you're right; I thought that IntersectionObserver::disconnect() would throw away its pending notifications, but it doesn't (I think that's a recent change).

So, yes, the null-check is the right thing to do.

I'm slightly concerned about this:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp?rcl=d2b7ed2ef23c46cce1d041ab2763132ca435a7bc&l=31

Is wrapWeakPersistent the right thing to do here?  Is it guaranteed that the ElementVisibilityObserver won't be gc-ed before IntersectionObserver::deliver runs?

It is possible. However when it happens, there's no need to deliver the observation to ElementVisibilityObserver, as the element it is observing is also gc-ed, i.e. the client doesn't need the observation anymore.

Also this will be safe as the closure created by WTF::bind will be cancelled when ElementVisibilityObserver is gc-ed:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/heap/PersistentTest.cpp?q=wrapweakpersistent&sq=package:chromium&dr=C&l=27

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

commit 0268543786283db4107d2362f338901d909545b1
Author: zqzhang <zqzhang@chromium.org>
Date: Tue Feb 07 16:25:19 2017

[Blink>Media] Fix a crash in autoplay caused by delayed visibility observation

There is a crash caused by delayed visibility observation after an
autoplay muted video gets unmuted, so that HTMLMediaElement tries to
stop the observer twice. However the pointer to the observer is set to
null on stop, thus a crash happens.

This CL adds null-check in
HTMLMediaElement::onVisibilityChangedForAutoplay() to avoid the crash.

BUG= 686458 

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

[modify] https://crrev.com/0268543786283db4107d2362f338901d909545b1/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp

We got ~3k reports on Stable:

https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Android%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%20CONTAINS%20%27ElementVisibilityObserver%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27blink%3A%3AElementVisibilityObserver%3A%3Astop%27%20AND%20product.Version%3D%2754.0.2840.85%27&ignore_case=true&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D

Requesting to merge into M57.

Should IntersectionObserver::disconnect throw away pending notifications? For the same reasons that we want it do, presumably web devs would too, right?

I agree. We should probably fix it in M58, but I prefer to merge the temporary fix to M57 as it is very small.

I see szager's new CL on IntersectionObserver is very short, maybe I revert my CL and merge the new one? Then we need to add a DCHECK() in HTMLMediaElement::onVisibilityChangedForAutoplay().

sgtm 👍

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

commit a00cedc6b6caf3fdf88bba8f4dba6b6fcda64a3e
Author: zqzhang <zqzhang@chromium.org>
Date: Tue Feb 07 20:47:49 2017

Revert of [Blink>Media] Fix a crash in autoplay caused by delayed visibility observation (patchset #1 id:1 of https://codereview.chromium.org/2671993002/ )

Reason for revert:
Decided to fix the issue in IntersectionObserver instead of HTMLMediaElement. This CL will be reverted and be superseded by the new fix.

Original issue's description:
> [Blink>Media] Fix a crash in autoplay caused by delayed visibility observation
>
> There is a crash caused by delayed visibility observation after an
> autoplay muted video gets unmuted, so that HTMLMediaElement tries to
> stop the observer twice. However the pointer to the observer is set to
> null on stop, thus a crash happens.
>
> This CL adds null-check in
> HTMLMediaElement::onVisibilityChangedForAutoplay() to avoid the crash.
>
> BUG= 686458 
>
> Review-Url: https://codereview.chromium.org/2671993002
> Cr-Commit-Position: refs/heads/master@{#448641}
> Committed: https://chromium.googlesource.com/chromium/src/+/0268543786283db4107d2362f338901d909545b1

TBR=mlamouri@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 686458 

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

[modify] https://crrev.com/a00cedc6b6caf3fdf88bba8f4dba6b6fcda64a3e/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp

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

commit 0591e6b5e4e2ded7d48edc18e20a6ed2466fd19a
Author: szager <szager@chromium.org>
Date: Tue Feb 07 23:55:08 2017

IntersectionObserver: discard notifications upon disconnect().

BUG= 686458 
R=ojan@chromium.org,zqzhang@chromium.org

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

[add] https://crrev.com/0591e6b5e4e2ded7d48edc18e20a6ed2466fd19a/third_party/WebKit/LayoutTests/intersection-observer/disconnect.html
[modify] https://crrev.com/0591e6b5e4e2ded7d48edc18e20a6ed2466fd19a/third_party/WebKit/Source/core/dom/IntersectionObserver.cpp
[modify] https://crrev.com/0591e6b5e4e2ded7d48edc18e20a6ed2466fd19a/third_party/WebKit/Source/web/tests/IntersectionObserverTest.cpp

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

ClusterFuzz has detected this issue as fixed in range 448729:448967.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=4802561975779328

Fuzzer: inferno_layout_test_unmodified
Job Type: mac_asan_content_shell
Platform Id: mac

Crash Type: UNKNOWN READ
Crash Address: 0x000000000010
Crash State:
  blink::ElementVisibilityObserver::stop
  blink::HTMLMediaElement::onVisibilityChangedForAutoplay
  blink::ElementVisibilityObserver::onVisibilityChanged
  
Sanitizer: address (ASAN)

Regressed: https://cluster-fuzz.appspot.com/revisions?job=mac_asan_content_shell&range=434473:434476
Fixed: https://cluster-fuzz.appspot.com/revisions?job=mac_asan_content_shell&range=448729:448967

Reproducer Testcase: https://cluster-fuzz.appspot.com/download/AMIfv97HOxznrs62wRFP4tIGB97lmiHlJRiQsj9zPikDHWh7w4v0Q99oJ-xVW6-c-0HICYVptYJHv4YkPNBJzeQsMi2WxZI_UNNu1RFuQThAKD1yWlgQhjUixgCb6-CXF54iEZcBpjZHbfNRDSt-p4ZzAhVR75RnYHSiOkVrD-VkfWWWdfa-Aps?testcase_id=4802561975779328


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 4802561975779328 is verified as fixed, so closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

We need to merge the fix (the last CL that szager landed) so reopen the issue now.

Before we approve merge to M57, could you please confirm CL by szager@ at #19 is well baked/verified in Canary and safe to merge to M57?

This change is very safe, no problems in canary

Approving merge for CL listed at #19 to M57 branch 2987 based on comment #25. Please merge ASAP. Thank you.

FYI It is merged at https://chromium.googlesource.com/chromium/src.git/+/13865a9236319d7ac82c2e61ccaa76f1c7225232

However there's no automatic message posted here yet. Will manually modify the merge tag if it doesn't happen in a few hours.

Per comment #27, this is already merged to M57. 

Issue 695390 has been merged into this issue.

We have made a bunch of changes on ClusterFuzz side, so resetting ClusterFuzz-Wrong label.