Closed
Bug 1182961
Opened 7 years ago
Closed 7 years ago
Use nsTHashTable::Iterator in netwerk/
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla42
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 5 obsolete files)
9.70 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
10.60 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
6.59 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
2.33 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
16.47 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
Because iterators are so much nicer than enumerate functions. There are fiftenn occurrences of EnumerateEntries() in netwerk/ to be dealt with.
![]() |
Assignee | |
Updated•7 years ago
|
Assignee: nobody → n.nethercote
![]() |
Assignee | |
Comment 1•7 years ago
|
||
Attachment #8639158 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Comment 2•7 years ago
|
||
Attachment #8639159 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Comment 3•7 years ago
|
||
Attachment #8639160 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Comment 4•7 years ago
|
||
Attachment #8639161 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Comment 5•7 years ago
|
||
Attachment #8639164 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Comment 6•7 years ago
|
||
(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.
![]() |
Assignee | |
Comment 7•7 years ago
|
||
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.
![]() |
Assignee | |
Comment 8•7 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9c3293e6097
![]() |
Assignee | |
Comment 9•7 years ago
|
||
Attachment #8640292 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8639158 -
Attachment is obsolete: true
Attachment #8639158 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Comment 10•7 years ago
|
||
Attachment #8640293 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8639159 -
Attachment is obsolete: true
Attachment #8639159 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Comment 11•7 years ago
|
||
Attachment #8640294 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8639160 -
Attachment is obsolete: true
Attachment #8639160 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Comment 12•7 years ago
|
||
Attachment #8640295 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8639161 -
Attachment is obsolete: true
Attachment #8639161 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Comment 13•7 years ago
|
||
Attachment #8640296 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8639164 -
Attachment is obsolete: true
Attachment #8639164 -
Flags: review?(michal.novotny)
Updated•7 years ago
|
Attachment #8640295 -
Flags: review?(michal.novotny) → review+
Comment 14•7 years ago
|
||
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.
Attachment #8640293 -
Flags: review?(michal.novotny) → review+
Updated•7 years ago
|
Attachment #8640296 -
Flags: review?(michal.novotny) → review+
![]() |
Assignee | |
Comment 15•7 years ago
|
||
Michal, you reviewed three patches but did not review (or, did not set a review result) on two. Was this deliberate?
Flags: needinfo?(michal.novotny)
![]() |
Assignee | |
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 16•7 years ago
|
||
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.
Flags: needinfo?(michal.novotny)
![]() |
Assignee | |
Comment 17•7 years ago
|
||
(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.
Flags: needinfo?(michal.novotny)
Updated•7 years ago
|
Attachment #8640294 -
Flags: review?(michal.novotny) → review+
Comment 18•7 years ago
|
||
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.
Attachment #8640292 -
Flags: review?(michal.novotny) → review+
Comment 19•7 years ago
|
||
(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.
Flags: needinfo?(michal.novotny)
Comment 20•7 years ago
|
||
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•7 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
![]() |
Assignee | |
Comment 22•7 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
![]() |
Assignee | |
Comment 24•7 years ago
|
||
> 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•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/322a318dafa8 https://hg.mozilla.org/integration/mozilla-inbound/rev/5b6b7472ff5a
![]() |
Assignee | |
Comment 26•7 years ago
|
||
(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•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/322a318dafa8 https://hg.mozilla.org/mozilla-central/rev/5b6b7472ff5a
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 28•7 years ago
|
||
Is this supposed to be RESOLVED-FIXED right now?
Flags: needinfo?(n.nethercote)
![]() |
Assignee | |
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Flags: needinfo?(n.nethercote)
Resolution: FIXED → ---
![]() |
Assignee | |
Updated•7 years ago
|
Keywords: leave-open
![]() |
Assignee | |
Comment 30•7 years ago
|
||
(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.
![]() |
Assignee | |
Comment 31•7 years ago
|
||
> ::: 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.
![]() |
Assignee | |
Comment 33•7 years ago
|
||
(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.
![]() |
Assignee | |
Comment 34•7 years ago
|
||
> 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•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2e4b173197e2 https://hg.mozilla.org/mozilla-central/rev/087e4e515ebe
![]() |
Assignee | |
Updated•7 years ago
|
Keywords: leave-open
![]() |
Assignee | |
Comment 37•7 years ago
|
||
(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•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9091e62b0646
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
![]() |
||
Comment 39•7 years ago
|
||
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.
Flags: needinfo?(n.nethercote)
![]() |
Assignee | |
Comment 40•7 years ago
|
||
(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.
Flags: needinfo?(n.nethercote)
![]() |
||
Comment 41•7 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•