New issue
Advanced search Search tips

Issue 891719 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 9
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 850624



Sign in to add a comment

NavigationPredictor: Same anchor element may be reported more than once

Project Member Reported by tbansal@chromium.org, Oct 3

Issue description

In third_party/blink/renderer/core/html/anchor_element_metrics_sender.h, the collection of HTMLAnchorElement is stored in a vector. However, the same anchor element may be added twice (possibly due to relayout?) by HTMLAnchorElement::InsertedInto() method.

When the duplicate anchor elements are sent to the browser process and their sizes merged together, it results in a target HREF having a visibility ratio of more than 1.0 which is wrong.

We should make sure that the same anchor element is only reported once to the browser process by checking for duplicates in anchor_element_metrics_sender.h.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 5

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

commit 70974c1a00b4ef70f2df877bddcc842ff30c91a2
Author: Tarun Bansal <tbansal@chromium.org>
Date: Fri Oct 05 15:36:33 2018

NavigationPredictor: Skip storage of duplicate anchor elements

The same anchor element may be stored more than once in
anchor_element_metrics_sender. Use a HashSet to avoid
duplicate storage. This allows the
navigation prediction logic in browser process is unable
to accurately determine the visibility ratio of a target
HREF.

Change-Id: I087e653f15094f8e32d66daa0d2cc6464d90f306
Bug:  891719 
Reviewed-on: https://chromium-review.googlesource.com/c/1258822
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597139}
[modify] https://crrev.com/70974c1a00b4ef70f2df877bddcc842ff30c91a2/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/70974c1a00b4ef70f2df877bddcc842ff30c91a2/third_party/blink/renderer/core/html/anchor_element_metrics.cc
[modify] https://crrev.com/70974c1a00b4ef70f2df877bddcc842ff30c91a2/third_party/blink/renderer/core/html/anchor_element_metrics.h
[modify] https://crrev.com/70974c1a00b4ef70f2df877bddcc842ff30c91a2/third_party/blink/renderer/core/html/anchor_element_metrics_sender.cc
[modify] https://crrev.com/70974c1a00b4ef70f2df877bddcc842ff30c91a2/third_party/blink/renderer/core/html/anchor_element_metrics_sender.h
[add] https://crrev.com/70974c1a00b4ef70f2df877bddcc842ff30c91a2/third_party/blink/renderer/core/html/anchor_element_metrics_sender_test.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Oct 9

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

commit a9086696f36b9abca6010186bc93e856fcf57b44
Author: Tarun Bansal <tbansal@chromium.org>
Date: Tue Oct 09 20:00:38 2018

Reenable DCHECKs in navigation predictor

Some of the DCHECKs were disabled due to a bug which was causing
same anchor element to be reported multiple times to the browser process.
Now, since that bug has been fixed, we can reenable some of the
DCHECKs in navigation predictor which ensure that the size occupied
by any target HREF (after aggregating across all anchor elements
that point to the same target) does not exceed 1.0.

Change-Id: I637724d57901c983df01760c749320f63813b3a8
Bug:  891719 
Reviewed-on: https://chromium-review.googlesource.com/c/1270480
Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598053}
[modify] https://crrev.com/a9086696f36b9abca6010186bc93e856fcf57b44/chrome/browser/navigation_predictor/navigation_predictor.cc

Status: Fixed (was: Started)
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 10

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

commit b53ce83d763b21d239669f642544f45b13ec3d0e
Author: Erik Chen <erikchen@chromium.org>
Date: Wed Oct 10 17:07:31 2018

Revert "Reenable DCHECKs in navigation predictor"

This reverts commit a9086696f36b9abca6010186bc93e856fcf57b44.

Reason for revert: DCHECKs are still being hit. They are causing flaky test failures. 

https://bugs.chromium.org/p/chromium/issues/detail?id=894092

Original change's description:
> Reenable DCHECKs in navigation predictor
> 
> Some of the DCHECKs were disabled due to a bug which was causing
> same anchor element to be reported multiple times to the browser process.
> Now, since that bug has been fixed, we can reenable some of the
> DCHECKs in navigation predictor which ensure that the size occupied
> by any target HREF (after aggregating across all anchor elements
> that point to the same target) does not exceed 1.0.
> 
> Change-Id: I637724d57901c983df01760c749320f63813b3a8
> Bug:  891719 
> Reviewed-on: https://chromium-review.googlesource.com/c/1270480
> Reviewed-by: Ryan Sturm <ryansturm@chromium.org>
> Commit-Queue: Tarun Bansal <tbansal@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#598053}

TBR=tbansal@chromium.org,ryansturm@chromium.org

Change-Id: I06c9a81264fee2380b89b090ca890be0f0d04743
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  891719 
Reviewed-on: https://chromium-review.googlesource.com/c/1273806
Reviewed-by: Erik Chen <erikchen@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598371}
[modify] https://crrev.com/b53ce83d763b21d239669f642544f45b13ec3d0e/chrome/browser/navigation_predictor/navigation_predictor.cc

Blocking: 850624

Sign in to add a comment