1182961 - Use nsTHashTable::Iterator in netwerk/

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Nicholas Nethercote [inactive]]

Because iterators are so much nicer than enumerate functions.

There are fiftenn occurrences of EnumerateEntries() in netwerk/ to be dealt with.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Use nsTHashtable::Iterator in CacheIndex

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use nsTHashtable::Iterator in CacheIndex

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Use nsTHashtable::Iterator in CacheIndex

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Use nsTHashtable::Iterator in CacheFileHandles

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Use nsTHashtable::Iterator in nsCookieService

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> Created attachment 8639158 [details] [diff] [review]
> (part 1) - Use nsTHashtable::Iterator in CacheIndex

There's something wrong with the change I've made to ProcessPendingOperations(). I'm getting assertion failures ('debugStats == mIndexStats') on try, e.g. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b3ebf57184c

I'm also getting some ASan failures (use after frees) in one of the later patches, as shown by https://treeherder.mozilla.org/#/jobs?repo=try&revision=fde1b4ed4724. I haven't looked closely at these failures yet.

This is surprising. I've done more than 100 of these enumeration-to-iteration conversions now and this is the first one I've had significant trouble with. They're generally very mechanical. There must be something subtle about this code that I'm unaware of.

Comment 7

[Nicholas Nethercote [inactive]]

The ASAN failures helped me understand what was going wrong. The problem was in cases where the CacheIndexEntryAutoManage class was used in conjunction with PL_DHASH_REMOVE. My patches changed those cases to use iter.Remove(), which changed the ordering of things.

- In the old code, CacheIndexEntryAutoManage was destroyed prior to the element being removed.

- In the new code, CacheIndexEntryAutoManage was destroyed after the element was removed.

I've changed my patches to use block-scoping around the CacheIndexEntryAutoManage instances to reinstate the original ordering. I have a try run going and if all goes well I'll post new patches once it finishes.

Comment 8

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9c3293e6097

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Use nsTHashtable::Iterator in CacheIndex

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use nsTHashtable::Iterator in CacheIndex

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Use nsTHashtable::Iterator in CacheIndex

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Use nsTHashtable::Iterator in CacheFileHandles

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Use nsTHashtable::Iterator in nsCookieService

Comment 14

[Michal Novotny [:michal]]

Comment on attachment 8640293 [details] [diff] [review]
(part 2) - Use nsTHashtable::Iterator in CacheIndex

Review of attachment 8640293 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheIndex.cpp
@@ +2318,5 @@
> +         LOGSHA1(entry->Hash())));
> +
> +    CacheIndexEntry* entry2 = mIndex.GetEntry(*entry->Hash());
> +    {
> +      CacheIndexEntryAutoManage emng(entry->Hash(), this);

iter.Remove() removes the entry from mTmpJournal. CacheIndexEntryAutoManage looks only into mIndex and mPendingUpdates so it's not necessary to put it into a block scope.

Comment 15

[Nicholas Nethercote [inactive]]

Michal, you reviewed three patches but did not review (or, did not set a review result) on two. Was this deliberate?

Comment 16

[Michal Novotny [:michal]]

I didn't understand what causes the problem that you described in comment #7. Ordering in the old code was unwanted behavior and methods DoNotSearchInIndex() and DoNotSearchInUpdates() are there to cope with it. It would be great to get rid of them now when the ordering has changed. It took me a while to figure out what causes the wrong statistics. CacheIndexEntryAutoManage hold just a pointer to the hash which is freed when the entry is released, so the hash is no longer valid in the destructor. I would prefer to change it so that CacheIndexEntryAutoManage copies the hash, then methods DoNotSearchIn...() can be removed and no block-scoping is needed.

Comment 17

[Nicholas Nethercote [inactive]]

(In reply to Michal Novotny (:michal) from comment #16)
> I would prefer to change it so that CacheIndexEntryAutoManage
> copies the hash, then methods DoNotSearchIn...() can be removed and no
> block-scoping is needed.

Can we do that as a follow-up? As it stands, these patches are just mechanical refactorings that don't require any netwerk/-specific knowledge. In contrast, changing CacheIndexEntryAutoManage in the way you suggest is scope creep that does require netwerk/-specific knowledge.

Comment 18

[Michal Novotny [:michal]]

Comment on attachment 8640292 [details] [diff] [review]
(part 1) - Use nsTHashtable::Iterator in CacheIndex

Review of attachment 8640292 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheIndex.cpp
@@ +1490,5 @@
>  
> +  for (auto iter = mPendingUpdates.Iter(); !iter.Done(); iter.Next()) {
> +    CacheIndexEntryUpdate* update = iter.Get();
> +
> +    LOG(("CacheFile::ProcessPendingOperations() [hash=%08x%08x%08x%08x%08x]",

Please fix the existing typo CacheFile -> CacheIndex.

Comment 19

[Michal Novotny [:michal]]

(In reply to Nicholas Nethercote [:njn] from comment #17)
> Can we do that as a follow-up? As it stands, these patches are just
> mechanical refactorings that don't require any netwerk/-specific knowledge.
> In contrast, changing CacheIndexEntryAutoManage in the way you suggest is
> scope creep that does require netwerk/-specific knowledge.

OK, I filed bug 1190839.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bff74cecc67c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffe0edb2aae7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b60b7c267cef
https://hg.mozilla.org/integration/mozilla-inbound/rev/6da154b43265
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcf6fd3ab9bb

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bff74cecc67c
https://hg.mozilla.org/mozilla-central/rev/ffe0edb2aae7
https://hg.mozilla.org/mozilla-central/rev/b60b7c267cef
https://hg.mozilla.org/mozilla-central/rev/6da154b43265
https://hg.mozilla.org/mozilla-central/rev/bcf6fd3ab9bb

Comment 22

[Nicholas Nethercote [inactive]]

I backed out all five patches due to problems:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7734e7adeb2

For bug 1191447 it looks like I screwed up ProcessPendingOperations().

For bug 1191907 it looks like I screwed up CacheIndex::WriteRecords().

I'll probably break these patches up into smaller pieces, so there is one enumeration-to-iteration change per patch, and then land them more gradually. I'll also look closely at the screwed-up changes and won't re-land them until I see what went wrong.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c797b5ac2442

Comment 24

[Nicholas Nethercote [inactive]]

> For bug 1191447 it looks like I screwed up ProcessPendingOperations().

I was able to reproduce the assertion just once, which wasn't enough to work out which patch is at fault. Part 1 definitely looks most likely, though.

> For bug 1191907 it looks like I screwed up CacheIndex::WriteRecords().

I was able to reproduce this fairly reliably, and I'm pretty sure this is right and part 2 is the problem.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/322a318dafa8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b6b7472ff5a

Comment 26

[Nicholas Nethercote [inactive]]

(In reply to Pulsebot from comment #25)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/322a318dafa8
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5b6b7472ff5a

I relanded parts 4 and 5 because I'm confident they weren't related to either of the problems.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/322a318dafa8
https://hg.mozilla.org/mozilla-central/rev/5b6b7472ff5a

Comment 28

[David Baron :dbaron:]

Is this supposed to be RESOLVED-FIXED right now?

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e4b173197e2

Comment 30

[Nicholas Nethercote [inactive]]

(In reply to Pulsebot from comment #29)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2e4b173197e2

That's part 3 re-landing, because I established locally that it isn't related to the problems.

Comment 31

[Nicholas Nethercote [inactive]]

> ::: netwerk/cache2/CacheIndex.cpp
> @@ +2318,5 @@
> > +         LOGSHA1(entry->Hash())));
> > +
> > +    CacheIndexEntry* entry2 = mIndex.GetEntry(*entry->Hash());
> > +    {
> > +      CacheIndexEntryAutoManage emng(entry->Hash(), this);
> 
> iter.Remove() removes the entry from mTmpJournal. CacheIndexEntryAutoManage
> looks only into mIndex and mPendingUpdates so it's not necessary to put it
> into a block scope.

It is necessary. I don't understand why because I don't really understand this code, but I have determined that the lack of the block scope around this CacheIndexEntryAutoManage is what caused bug 1191447.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/087e4e515ebe

Comment 33

[Nicholas Nethercote [inactive]]

(In reply to Pulsebot from comment #32)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/087e4e515ebe

That's part 1 re-landing, because I established locally that it isn't related to the problems. In other words, both the problems came from part 2.

Comment 34

[Nicholas Nethercote [inactive]]

> I have determined that the lack of the block scope around
> this CacheIndexEntryAutoManage is what caused bug 1191447.

And now I've confirmed that it also caused by 1191907.

I'll re-land part 2 with the extra scope in the next day or two. I will wait so that it'll end up in a different Nightly to parts 1 and 3, just to be cautious.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e4b173197e2
https://hg.mozilla.org/mozilla-central/rev/087e4e515ebe

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9091e62b0646

Comment 37

[Nicholas Nethercote [inactive]]

(In reply to Pulsebot from comment #36)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9091e62b0646

That's part 2. All the patches have now re-landed.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9091e62b0646

Comment 39

[Honza Bambas (:mayhemer)]

Nathan, how long do you need to verify this is fixed before I land bug 1082735?  It touches the same code, so regression hunt would be harder.

Comment 40

[Nicholas Nethercote [inactive]]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #39)
> Nathan, how long do you need to verify this is fixed before I land bug
> 1082735?  It touches the same code, so regression hunt would be harder.

I'm Nick, not Nathan :)

I'm confident it's fixed. I was able to reproduce the problems locally and the new versions didn't trigger them. Also, both problems were reported pretty much as soon as the code hit Nightly last time. So as long as it's been present in Nightly for a full day without complaint you should be ok. I'm not sure when Nightly is built from mozilla-central, but given that it landed on mozilla-central early yesterday, I think 24 hours from now should be safe, and 48 hours should be super-safe.

Sorry for causing the delay.

Comment 41

[Honza Bambas (:mayhemer)]

(In reply to Nicholas Nethercote [:njn] from comment #40)
> (In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #39)
> > Nathan, how long do you need to verify this is fixed before I land bug
> > 1082735?  It touches the same code, so regression hunt would be harder.
> 
> I'm Nick, not Nathan :)
> 
> I'm confident it's fixed. I was able to reproduce the problems locally and
> the new versions didn't trigger them. Also, both problems were reported
> pretty much as soon as the code hit Nightly last time. So as long as it's
> been present in Nightly for a full day without complaint you should be ok.
> I'm not sure when Nightly is built from mozilla-central, but given that it
> landed on mozilla-central early yesterday, I think 24 hours from now should
> be safe, and 48 hours should be super-safe.
> 
> Sorry for causing the delay.

Sorry for name mismatch :D  Thanks, I'll land my patch in few days.