[FastTextAutosizer] Keep m_blocksForFingerprint and m_fingerprints in sync

[FastTextAutosizer] Keep m_blocksForFingerprint and m_fingerprints in sync

This patch keeps m_blocksForFingerprint in sync with m_fingerprints when
adding a new entry to m_fingerprints. Here's a quick summary about
these maps:
m_blocksForFingerprint maps Fingerprint -> list of RenderBlocks
m_fingerprints maps RenderObject -> Fingerprint

Previously, when adding a block to m_fingerprints we could neglect to
update m_blocksForFingerprint. By removing entries from
m_blocksForFingerprint before registering a new block fingerprint, we
prevent a crash on [1].

[1] http://www.chicagotribune.com/news/sns-wp-blm-news-bc-inventor01-20140301,0,255272.story

TEST=Added an assert that crashes all over our layouttests without this fix. Manually verified [1] no longer crashes.
BUG=348463
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168533

[pdr., 2014-03-04 07:15:58]

Still working on a test and understanding how this happens. This does fix the
crash (unlike the previous patch).

In writing this test I wrote a little script to help debug the supercluster and
fingerprint maps that may be useful in the future:
https://codereview.chromium.org/186443002.

[skobes, 2014-03-04 18:50:29]

lgtm

[pdr., 2014-03-05 03:35:35]

On 2014/03/04 18:50:29, skobes wrote:
> lgtm

PTAL.

I've updated the patch to be faster. Instead of calling remove(), I'm explicitly
removing a potential double-entry in m_blocksForFingerprint that leads to a
use-after-free.

I give up trying to get that page to reproduce (it's not deterministic due to
load times), but I added a new check "FingerprintMapper::mapsAreConsistent()"
that assert that m_blocksForFingerprint is sane. Without the fix in this patch,
we crash on many layouttests. It's sort of lame to assert the inverse of the fix
and call it a success, but I think the new mapsAreConsistent checks bake in our
assumptions.

[skobes, 2014-03-05 04:12:21]

https://codereview.chromium.org/186453002/diff/20002/Source/core/rendering/Fa...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/186453002/diff/20002/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:677: return true;
It's a bit silly to have a return value that is always true - just return void
and call it assertMapsAreConsistent.

https://codereview.chromium.org/186453002/diff/20002/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:688:
blocksIter->value->remove(toRenderBlock(renderer));
I think I liked it better when this just called FingerprintMapper::remove.

If blocksIter->value is empty after this call, don't we want to remove it from
m_blocksForFingerprint?

[pdr., 2014-03-05 04:20:31]

Updated!

https://codereview.chromium.org/186453002/diff/20002/Source/core/rendering/Fa...
File Source/core/rendering/FastTextAutosizer.cpp (right):

https://codereview.chromium.org/186453002/diff/20002/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:677: return true;
On 2014/03/05 04:12:22, skobes wrote:
> It's a bit silly to have a return value that is always true - just return void
> and call it assertMapsAreConsistent.

Fair point. I was jumping through silly hoops to avoid #ifndef NDEBUG checks.

Fixed.

https://codereview.chromium.org/186453002/diff/20002/Source/core/rendering/Fa...
Source/core/rendering/FastTextAutosizer.cpp:688:
blocksIter->value->remove(toRenderBlock(renderer));
On 2014/03/05 04:12:22, skobes wrote:
> I think I liked it better when this just called FingerprintMapper::remove.
> 
> If blocksIter->value is empty after this call, don't we want to remove it from
> m_blocksForFingerprint?

Yeah, I agree. This will have one unnecessary hash lookup (we remove an entry
from m_fingerprints and immediately set it below) but I think it's worth it for
code maintainability.

Done.

[skobes, 2014-03-05 06:58:14]

lgtm

[pdr., 2014-03-05 07:32:15]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-05 07:33:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/186453002/50001

[pdr., 2014-03-05 19:36:05]

On 2014/03/05 07:33:08, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/pdr%40chromium.org/186453002/50001

win bot is hosed. Adding notry=true

[pdr., 2014-03-05 19:36:24]

The CQ bit was unchecked by pdr@chromium.org

[pdr., 2014-03-05 19:36:32]

The CQ bit was checked by pdr@chromium.org

[pdr., 2014-03-05 19:36:33]

The CQ bit was unchecked by pdr@chromium.org

[pdr., 2014-03-05 19:36:34]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-03-05 20:57:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pdr@chromium.org/186453002/50001

[commit-bot: I haz the power, 2014-03-05 21:04:22]

Change committed as 168533