Closed Bug 1434206 Opened 8 years ago Closed 7 years ago

Intermittent marionette.py | application crashed [@ mozilla::safebrowsing::HashStore::ApplyUpdate(mozilla::safebrowsing::TableUpdate&)]

Categories

(Toolkit :: Safe Browsing, defect, P1)

Product:
Toolkit
Component:
Safe Browsing
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: francois)

References

Details

(5 keywords, Whiteboard: [adv-main62-])

Crash Data

Attachments

(16 files)

Bug 1434206 - Remove unused and undefined functions.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Add const to members and functions that can take it.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Clear the current table when protocol parser is done.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Use a TableUpdateV2 param in ApplyUpdate().
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Don't cache gethash response if we failed to apply results.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Assert that gethash processing happens on the right thread.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Keep TableUpdate objects in smart pointers.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Make TableUpdate objects const as much as possible.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Replace a pointer with a reference.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Add const to functions and members that can take it.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Keep LookupCache objects in smart pointers.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Make LookupCache objects const as much as possible.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Clarify when the lookupcache arrays should be cleared.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Keep CacheResult objects in smart pointers.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Make DBSserviceWorkerProxy as const as possible.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1434206 - Keep LookupResult objects in smart pointers.
59 bytes, text/x-review-board-request
gcp
: review+
Details
14:37:14 INFO - Crash reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS 14:37:14 INFO - Crash address: 0xffffffff8bae745c 14:37:14 INFO - Process uptime: 3 seconds 14:37:14 INFO - 14:37:14 INFO - Thread 51 (crashed) 14:37:14 INFO - 0 XUL!mozilla::safebrowsing::HashStore::ApplyUpdate(mozilla::safebrowsing::TableUpdate&) [nsTArray.h:8e4a39d65561 : 538 + 0x4] 14:37:14 INFO - rax = 0x00000001375a2000 rdx = 0x000000081515150d 14:37:14 INFO - rcx = 0x00000000e5e5e5e5 rbx = 0x0000000000000e38 14:37:14 INFO - rsi = 0x000000218bae7434 rdi = 0x000000005a2824a9 14:37:14 INFO - rbp = 0x0000000131b99610 rsp = 0x0000000131b99590 14:37:14 INFO - r8 = 0x000000008007000e r9 = 0xfffffffffffff000 14:37:14 INFO - r10 = 0x00000000ffffffff r11 = 0x0000000000020008 14:37:14 INFO - r12 = 0x00000001370d4a00 r13 = 0x0000000000000000 14:37:14 INFO - r14 = 0x0000000000000000 r15 = 0x0000000137521fe8 14:37:14 INFO - rip = 0x000000010fbe84a7 14:37:14 INFO - Found by: given as instruction pointer in context 14:37:14 INFO - 1 XUL!mozilla::safebrowsing::Classifier::UpdateHashStore(nsTArray<mozilla::safebrowsing::TableUpdate*>*, nsTSubstring<char> const&) [Classifier.cpp:8e4a39d65561 : 1246 + 0x8] 14:37:14 INFO - rbx = 0x000000000000000e rbp = 0x0000000131b99700 14:37:14 INFO - rsp = 0x0000000131b99620 r12 = 0x00000001370d4a00 14:37:14 INFO - r13 = 0x00000001314d1070 r14 = 0x0000000000000000 14:37:14 INFO - r15 = 0x0000000000000000 rip = 0x000000010fbf7d1d 14:37:14 INFO - Found by: call frame info 14:37:14 INFO - 2 XUL!mozilla::safebrowsing::Classifier::ApplyUpdatesBackground(nsTArray<mozilla::safebrowsing::TableUpdate*>*, nsTSubstring<char>&) [Classifier.cpp:8e4a39d65561 : 831 + 0x10] 14:37:14 INFO - rbx = 0x000000012c9d4c40 rbp = 0x0000000131b997b0 14:37:14 INFO - rsp = 0x0000000131b99710 r12 = 0x0000000131b99770 14:37:14 INFO - r13 = 0x00000001314d1070 r14 = 0x0000000000000006 14:37:14 INFO - r15 = 0x0000000000000005 rip = 0x000000010fbf76fe 14:37:14 INFO - Found by: call frame info 14:37:14 INFO - 3 XUL!mozilla::detail::RunnableFunction<mozilla::safebrowsing::Classifier::AsyncApplyUpdates(nsTArray<mozilla::safebrowsing::TableUpdate*>*, std::__1::function<void (nsresult)> const&)::$_1>::Run() [Classifier.cpp:8e4a39d65561 : 747 + 0xb] 14:37:14 INFO - rbx = 0x0000000121e89b00 rbp = 0x0000000131b99860 14:37:14 INFO - rsp = 0x0000000131b997c0 r12 = 0x00000001370d36d0 14:37:14 INFO - r13 = 0x0000000000000000 r14 = 0x0000000131b997c0 14:37:14 INFO - r15 = 0x0000000110bd0350 rip = 0x000000010fc29126 14:37:14 INFO - Found by: call frame info 14:37:14 INFO - 4 XUL!nsThread::ProcessNextEvent(bool, bool*) [nsThread.cpp:8e4a39d65561 : 1040 + 0x6] 14:37:14 INFO - rbx = 0x000000012fbacd01 rbp = 0x0000000131b99dd0 14:37:14 INFO - rsp = 0x0000000131b99870 r12 = 0x0000000000000001 14:37:14 INFO - r13 = 0x0000000000000000 r14 = 0x000000012fbacd40 14:37:14 INFO - r15 = 0x0000000131b998b0 rip = 0x000000010c75d693 14:37:14 INFO - Found by: call frame info 14:37:14 INFO - 5 XUL!NS_ProcessNextEvent(nsIThread*, bool) [nsThreadUtils.cpp:8e4a39d65561 : 517 + 0xd] 14:37:14 INFO - rbx = 0x0000000000000001 rbp = 0x0000000131b99df0 14:37:14 INFO - rsp = 0x0000000131b99de0 r12 = 0x00000001314dd160 14:37:14 INFO - r13 = 0x0000000000000000 r14 = 0x00000001314dd140 14:37:14 INFO - r15 = 0x000000012fbacd40 rip = 0x000000010c766eff 14:37:14 INFO - Found by: call frame info 14:37:14 INFO - 6 XUL!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [MessagePump.cpp:8e4a39d65561 : 364 + 0xd] 14:37:14 INFO - rbx = 0x000000012fbe1d20 rbp = 0x0000000131b99e40 14:37:14 INFO - rsp = 0x0000000131b99e00 r12 = 0x00000001314dd160 14:37:14 INFO - r13 = 0x0000000000000000 r14 = 0x00000001314dd140 14:37:14 INFO - r15 = 0x000000012fbacd40 rip = 0x000000010cbebdca 14:37:14 INFO - Found by: call frame info 14:37:14 INFO - 7 XUL!MessageLoop::Run() [message_loop.cc:8e4a39d65561 : 326 + 0x8] 14:37:14 INFO - rbx = 0x000000012fbe1d20 rbp = 0x0000000131b99e70 14:37:14 INFO - rsp = 0x0000000131b99e50 r12 = 0x0000000000011a03 14:37:14 INFO - r13 = 0x00000000000008ff r14 = 0x000000012fbacd60 14:37:14 INFO - r15 = 0x000000012fbacd40 rip = 0x000000010cbb46b9 14:37:14 INFO - Found by: call frame info 14:37:14 INFO - 8 XUL!nsThread::ThreadFunc(void*) [nsThread.cpp:8e4a39d65561 : 423 + 0x5] 14:37:14 INFO - rbx = 0x000000012fbe1d20 rbp = 0x0000000131b99ec0 14:37:14 INFO - rsp = 0x0000000131b99e80 r12 = 0x0000000000011a03 14:37:14 INFO - r13 = 0x00000000000008ff r14 = 0x000000012fbacd60 14:37:14 INFO - r15 = 0x000000012fbacd40 rip = 0x000000010c75b89b 14:37:14 INFO - Found by: call frame info 14:37:14 INFO - 9 libnss3.dylib!_pt_root [ptthread.c:8e4a39d65561 : 201 + 0x3] 14:37:14 INFO - rbx = 0x000000012e291a10 rbp = 0x0000000131b99ef0 14:37:14 INFO - rsp = 0x0000000131b99ed0 r12 = 0x0000000000011a03 14:37:14 INFO - r13 = 0x00000000000008ff r14 = 0x0000000131b9a000 14:37:14 INFO - r15 = 0x0000000000000000 rip = 0x000000010c4441e3 14:37:14 INFO - Found by: call frame info 14:37:14 INFO - 10 libsystem_pthread.dylib!_pthread_body + 0x83 14:37:14 INFO - rbx = 0x0000000131b9a000 rbp = 0x0000000131b99f10 14:37:14 INFO - rsp = 0x0000000131b99f00 r12 = 0x0000000000011a03 14:37:14 INFO - r13 = 0x00000000000008ff r14 = 0x000000012e291a10 14:37:14 INFO - r15 = 0x000000010c444110 rip = 0x00007fff869d905a 14:37:14 INFO - Found by: call frame info 14:37:14 INFO - 11 libsystem_pthread.dylib!_pthread_start + 0xb0 14:37:14 INFO - rbp = 0x0000000131b99f50 rsp = 0x0000000131b99f20 14:37:14 INFO - rip = 0x00007fff869d8fd7 14:37:14 INFO - Found by: previous frame's frame pointer 14:37:14 INFO - 12 libsystem_pthread.dylib!thread_start + 0xd 14:37:14 INFO - rbp = 0x0000000131b99f78 rsp = 0x0000000131b99f60 14:37:14 INFO - rip = 0x00007fff869d63ed 14:37:14 INFO - Found by: previous frame's frame pointer 14:37:14 INFO - 13 libnss3.dylib + 0x144110 14:37:14 INFO - rsp = 0x0000000131b9a030 rip = 0x000000010c444110 14:37:14 INFO - Found by: stack scanning The crash address doesn't look safe, so I'm going to mark it as security related for now.
Group: toolkit-core-security
Keywords: csectype-uaf, sec-high
Priority: P5 → --
(In reply to Treeherder Bug Filer from comment #0) > As it looks like it needs a fix via bug 1361598. That bug was filed on a prolific _Mac_ crash that still seems to be gone. I re-closed that one WFM because I think it's still gone. If there's something new we can deal with it here. The new Windows crash I see in crash-stats (bp-bedbf60d-fc46-4fde-9947-f901d0180205) is an Exec access violation, doesn't look as much like a UAF as the registers (rcx) here. In both rcx and rdx, though, is the value 0x000007fee8f3dea8 -- is that last bit "dead" or just a coincidence? Do we have something (or does the compiler) mark freed memory that way? It's not quite our frame poisoning address (which wouldn't apply to this HashStore anyway). On the other hand comment 0 is a Mac crash, and the Mac crashes in bug 1361598 also showed the UAF marker in rcx. Interesting that OS X 10.11 is the highest Mac OS that shows this crash and this treeherder job is OS X 10.10. Sierra and High Sierra don't seem to get this crash for some reason? Maybe this _is_ the same as bug 1361598 and the windows crash is the oddball something else. I'm worried about that Exec-AV though.
Crash Signature: [@ mozilla::safebrowsing::HashStore::ApplyUpdate(mozilla::safebrowsing::TableUpdate&)] → [@ mozilla::safebrowsing::HashStore::ApplyUpdate]
Flags: needinfo?(francois)
This is internal table update code. The Windows crash and the older Mac crashes were all in "v2" code, which is currently used for tracking protection and flash blocking lists (the Google-supplied lists use the v4 protocol code). Updating these lists can't be directly triggered by web content and the list data comes from trusted sources. Lowering the severity to sec-moderate. This is so unlikely to be exploitable that it's more useful to make this bug public. We may need to close it incomplete or worksforme (like bug 1361598) if we can't make progress and it's not worth tracking as a topcrash.
Flags: needinfo?(francois)
Keywords: sec-high → sec-moderate
Group: toolkit-core-security
Priority: -- → P3
https://wiki.mozilla.org/Bug_Triage#Intermittent_Test_Failure_Cleanup
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE
I looked at these two Win 8.1 crashes: https://crash-stats.mozilla.com/report/index/7173d8e4-5e3e-47a4-8264-4afbf0180325 https://crash-stats.mozilla.com/report/index/b5bc09ac-d1df-4590-b7e0-7ed9b0180325 and saw that the crash line is the closing of the scope for this function: https://hg.mozilla.org/releases/mozilla-release/annotate/3db9e3d52b17/toolkit/components/url-classifier/HashStore.cpp#l655 which suggests that we crash when deallocating the TableUpdate object. The destructor for both TableUpdateV2 and TableUpdateV4 are empty: https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/toolkit/components/url-classifier/HashStore.h#33 however there is a dependent string (removed in my patch for bug 1438671) that might cause trouble: https://searchfox.org/mozilla-central/rev/57bbc1ac58816dc054df242948f3ecf75e12df5f/toolkit/components/url-classifier/HashStore.h#146 when we de-allocate mPrefixesMap.
See Also: → 1438671
https://crash-stats.mozilla.com/report/index/bedbf60d-fc46-4fde-9947-f901d0180205 is an EXEC error on windows, in 58.0.1, and appears to be crashing in the call to Merge() for AddPrefixes. the updateV2's TableUpdate's _vfptr is all 0's. mTable has in the nsString and mData of 0x000007fee8f3dea8, and an mLength of 0xe8f3dea8 (note!) This implies to me either an overwrite, or a use-after-reallocate. I see no e5e5 crashes. Crashes go back a long way, and are 95% Mac, which may also point to a thread-timing issue. The crash address, 0x22dd37c0 is the address of 'update' in UpdateHashStore; somehow it tried to execute that address, if you believe the dump/stack. All these crashes appear to be sec issues, and EXEC random addresses are bad, and the INVALID_ADDRESS is still concerning on mac.
Group: toolkit-core-security
Keywords: sec-moderate → sec-high
Assignee: nobody → francois
Status: RESOLVED → REOPENED
Priority: P3 → P1
Resolution: INCOMPLETE → ---
Un-hiding again at Francois' request. This code is only accessible to the SafeBrowsing provider (Google) and any TLS MITM (which you either trust or you're already having a bad day).
Group: toolkit-core-security
Keywords: sec-high → csectype-bounds, sec-moderate
GCP, I apologize for the large patchset. The Safe Browsing yak was in need of a haircut. The patch which should help with the memory mismanagement crashes mentioned in this bug is the one titled "Keep TableUpdate objects in smart pointers."
Comment on attachment 8981659 [details] Bug 1434206 - Remove unused and undefined functions. https://reviewboard.mozilla.org/r/247778/#review254018
Attachment #8981659 - Flags: review?(gpascutto) → review+
Comment on attachment 8981660 [details] Bug 1434206 - Add const to members and functions that can take it. https://reviewboard.mozilla.org/r/247780/#review254020 ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:918 (Diff revision 1) > nsUrlClassifierDBServiceWorker::CacheResultToTableUpdate(CacheResult* aCacheResult, > TableUpdate* aUpdate) > { > auto tuV2 = TableUpdate::Cast<TableUpdateV2>(aUpdate); > if (tuV2) { > - auto result = CacheResult::Cast<CacheResultV2>(aCacheResult); > + const CacheResultV2* result = CacheResult::Cast<CacheResultV2>(aCacheResult); Not sure you care but "const auto" is a thing.
Attachment #8981660 - Flags: review?(gpascutto) → review+
Comment on attachment 8981661 [details] Bug 1434206 - Clear the current table when protocol parser is done. https://reviewboard.mozilla.org/r/247782/#review254024
Attachment #8981661 - Flags: review?(gpascutto) → review+
Comment on attachment 8981662 [details] Bug 1434206 - Use a TableUpdateV2 param in ApplyUpdate(). https://reviewboard.mozilla.org/r/247784/#review254026
Attachment #8981662 - Flags: review?(gpascutto) → review+
Comment on attachment 8981663 [details] Bug 1434206 - Don't cache gethash response if we failed to apply results. https://reviewboard.mozilla.org/r/247786/#review254030 ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:908 (Diff revision 1) > result->table.get())); > } > } > > - mClassifier->ApplyFullHashes(&updates); > + rv = mClassifier->ApplyFullHashes(&updates); > + NS_ENSURE_SUCCESS(rv, rv); I think it would be strongly preferable to put the mLastResults = inside an explicit if (NS_SUCCEEDED()) and maybe just do a return rv at the end here (thus also getting rid of the deprecated NS_ENSURE_SUCCESS).
Attachment #8981663 - Flags: review?(gpascutto) → review+
Comment on attachment 8981664 [details] Bug 1434206 - Assert that gethash processing happens on the right thread. https://reviewboard.mozilla.org/r/247788/#review254036
Attachment #8981664 - Flags: review?(gpascutto) → review+
Comment on attachment 8981665 [details] Bug 1434206 - Keep TableUpdate objects in smart pointers. https://reviewboard.mozilla.org/r/247790/#review254052 ::: toolkit/components/url-classifier/nsUrlClassifierDBService.h:30 (Diff revision 1) > > #include "Entries.h" > #include "LookupCache.h" > +#include "HashStore.h" > > // GCC < 6.1 workaround, see bug 1329593 Almost. ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:422 (Diff revision 1) > NS_ERROR("Unable to open SafeBrowsing database"); > return NS_ERROR_FAILURE; > } > > mUpdateStatus = NS_OK; > + mTableUpdates.Clear(); It feels wrong this is in BeginUpdates. Should we not assert/ensure this is empty at the ending/erroring out of a previous update (and so certainly here)? Else we'll be keeping the update in memory.
Attachment #8981665 - Flags: review?(gpascutto)
Comment on attachment 8981667 [details] Bug 1434206 - Replace a pointer with a reference. https://reviewboard.mozilla.org/r/247794/#review254062
Attachment #8981667 - Flags: review?(gpascutto) → review+
Comment on attachment 8981666 [details] Bug 1434206 - Make TableUpdate objects const as much as possible. https://reviewboard.mozilla.org/r/247792/#review254070 ::: toolkit/components/url-classifier/HashStore.h:60 (Diff revision 1) > > const nsCString mTable; > }; > > typedef nsTArray<RefPtr<TableUpdate>> TableUpdateArray; > +typedef nsTArray<RefPtr<const TableUpdate>> ConstTableUpdateArray; I can't really remember seeing this pattern before, which makes me suspect there's a better way to achieve this than to have 2 seperate types for the same thing. I think many consumers just want to take some kind of const_iterator to inspect the innermost elements.
Comment on attachment 8981668 [details] Bug 1434206 - Add const to functions and members that can take it. https://reviewboard.mozilla.org/r/247796/#review254072
Attachment #8981668 - Flags: review?(gpascutto) → review+
Comment on attachment 8981669 [details] Bug 1434206 - Keep LookupCache objects in smart pointers. https://reviewboard.mozilla.org/r/247798/#review254076 ::: toolkit/components/url-classifier/tests/gtest/TestPerProviderDirectory.cpp:77 (Diff revision 1) > // used as the private store. > - VerifyPrivateStorePath<LookupCacheV2>("goog-phish-shavar", "google", rootDir, false); > + { > + nsAutoCString table("goog-phish-shavar"); > + nsAutoCString provider("google"); > + RefPtr<LookupCacheV2> lc = new LookupCacheV2(table, provider, rootDir); > + VerifyPrivateStorePath<LookupCacheV2>(lc, table, provider, rootDir, false); Not really clear why you changed this as you now are required to create that dummy LookupCache everywhere (duplicating a lot of code)? ::: toolkit/components/url-classifier/tests/gtest/TestUrlClassifierTableUpdateV4.cpp:258 (Diff revision 1) > nsCOMPtr<nsIFile> file; > NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(file)); > file->AppendNative(GTEST_SAFEBROWSING_DIR); > > RunTestInNewThread([&] () -> void { > - LookupCacheV4 cache(nsCString(GTEST_TABLE), EmptyCString(), file); > + RefPtr<LookupCacheV4> cache = new LookupCacheV4(nsCString(GTEST_TABLE), This didn't need to be changed? It was not a pointer.
Attachment #8981669 - Flags: review?(gpascutto) → review+
Comment on attachment 8981670 [details] Bug 1434206 - Make LookupCache objects const as much as possible. https://reviewboard.mozilla.org/r/247800/#review254084
Attachment #8981670 - Flags: review?(gpascutto) → review+
Comment on attachment 8981660 [details] Bug 1434206 - Add const to members and functions that can take it. https://reviewboard.mozilla.org/r/247780/#review254020 > Not sure you care but "const auto" is a thing. Cool, I didn't realize that. This specific line changes again in a later patch, so I'll leave it as it is.
Comment on attachment 8981665 [details] Bug 1434206 - Keep TableUpdate objects in smart pointers. https://reviewboard.mozilla.org/r/247790/#review254052 > Almost. Sorry, I'm confused. Are you suggesting a change here?
NI? for the question in comment 41.
Flags: needinfo?(gpascutto)
Comment on attachment 8981669 [details] Bug 1434206 - Keep LookupCache objects in smart pointers. https://reviewboard.mozilla.org/r/247798/#review254076 > Not really clear why you changed this as you now are required to create that dummy LookupCache everywhere (duplicating a lot of code)? I ran into problems after making LookupCache RefPtr'able. There was a compiler error about the destructor being private and I didn't see a way to make that work as a regular pointer or as a stack object. > This didn't need to be changed? It was not a pointer. I changed that for the same reason as the previous one. I couldn't get it to compile.
Comment on attachment 8981666 [details] Bug 1434206 - Make TableUpdate objects const as much as possible. https://reviewboard.mozilla.org/r/247792/#review254548 ::: toolkit/components/url-classifier/HashStore.h:60 (Diff revision 1) > > const nsCString mTable; > }; > > typedef nsTArray<RefPtr<TableUpdate>> TableUpdateArray; > +typedef nsTArray<RefPtr<const TableUpdate>> ConstTableUpdateArray; Yeah, it feels weird, but there are really two classes of consumers for this: - those who will be modifying the underlying array elements - those who are only reading from the array objects I wanted to encode the kind of use directly into the type so that we can make these assumptions explicit. A `const_iterator` just means you're not adding/removing anything to the array or changing the objects that the elements point to. It doesn't prevent the consumer from changing the underlying `TableUpdate` object. In other words, I don't really have a better idea for how to do this.
>Sorry, I'm confused. Are you suggesting a change here? No, I'm saying that hack can almost be removed as we bumped our GCC requirement recently. I needed to update to 6.4 but I'm not sure if 6.1 is supported or not.
Flags: needinfo?(gpascutto)
>A `const_iterator` just means you're not adding/removing anything to the array or changing the objects that the elements point to. Yes, I was figuratively speaking, not the literal thing from C++. You'd want to take an iterator-like object in those functions that gives you access to the underlying elements (so after the index and deref) but gives them as const references.
Comment on attachment 8981671 [details] Bug 1434206 - Clarify when the lookupcache arrays should be cleared. https://reviewboard.mozilla.org/r/247802/#review254794
Attachment #8981671 - Flags: review?(gpascutto) → review+
Comment on attachment 8981672 [details] Bug 1434206 - Keep CacheResult objects in smart pointers. https://reviewboard.mozilla.org/r/247804/#review254798 ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:872 (Diff revision 1) > LOG(("Active tables: %s", s.get())); > } > > ConstTableUpdateArray updates; > > - for (uint32_t i = 0; i < resultsPtr->Length(); i++) { > + for (auto iter = aResults.cbegin(); iter != aResults.cend(); ++iter) { You can use a range based for by using const auto& I think.
Attachment #8981672 - Flags: review?(gpascutto) → review+
Comment on attachment 8981673 [details] Bug 1434206 - Make DBSserviceWorkerProxy as const as possible. https://reviewboard.mozilla.org/r/247806/#review254800 ::: toolkit/components/url-classifier/nsUrlClassifierProxies.h:167 (Diff revision 1) > NS_DECL_NSIRUNNABLE > private: > - RefPtr<nsUrlClassifierDBServiceWorker> mTarget; > - > - nsCString mSpec; > - nsCString mTables; > + const RefPtr<nsUrlClassifierDBServiceWorker> mTarget; > + const nsCString mSpec; > + const nsCString mTables; > + mozilla::safebrowsing::LookupResultArray* const mResults; Looks like another candidate for RefPtr perhaps? Seems to be the odd raw pointer out here?
Attachment #8981673 - Flags: review?(gpascutto) → review+
Comment on attachment 8981673 [details] Bug 1434206 - Make DBSserviceWorkerProxy as const as possible. https://reviewboard.mozilla.org/r/247806/#review254800 > Looks like another candidate for RefPtr perhaps? Seems to be the odd raw pointer out here? Indeed :) https://reviewboard.mozilla.org/r/247808/diff/1#index_header
(In reply to Gian-Carlo Pascutto [:gcp] from comment #46) > >A `const_iterator` just means you're not adding/removing anything to the array or changing the objects that the elements point to. > > Yes, I was figuratively speaking, not the literal thing from C++. You'd want > to take an iterator-like object in those functions that gives you access to > the underlying elements (so after the index and deref) but gives them as > const references. I see, creating a const wrapper of some sort. Are you ok with my filing a follow-up bug for this? I agree it could be improved, but I'd like to get this crash fix in as soon as possible and also clean up *PrefixSet* in a similar way to hopefully resolve bug 1362761.
Flags: needinfo?(gpascutto)
Comment on attachment 8981674 [details] Bug 1434206 - Keep LookupResult objects in smart pointers. https://reviewboard.mozilla.org/r/247808/#review254824 Looks good. Perhaps const auto trick with range based for can also be used in a lot of places here.
Attachment #8981674 - Flags: review?(gpascutto) → review+
Comment on attachment 8981666 [details] Bug 1434206 - Make TableUpdate objects const as much as possible. https://reviewboard.mozilla.org/r/247792/#review254830 OK to take as is. Don't worry anything not so clean you introduce will probably be cleaned up by your successor in 8 years :-) :-)
Attachment #8981666 - Flags: review?(gpascutto) → review+
Flags: needinfo?(gpascutto)
GCP, not sure if this last patch is showing up on your review queue because the flag looks weird on MozReview. Setting NI? in case it's not there. I've addressed all of your review feedback in my last push.
Flags: needinfo?(gpascutto)
See Also: → 1466279
See Also: 1438671 →
Comment on attachment 8981665 [details] Bug 1434206 - Keep TableUpdate objects in smart pointers. https://reviewboard.mozilla.org/r/247790/#review255852 ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:912 (Diff revisions 1 - 2) > } > } > > rv = mClassifier->ApplyFullHashes(updates); > - NS_ENSURE_SUCCESS(rv, rv); > + if (NS_SUCCEEDED(rv)) { > - mLastResults = Move(resultsPtr); > + mLastResults = Move(resultsPtr); dev.platform says we should be using std::move now.
Attachment #8981665 - Flags: review?(gpascutto) → review+
I think I got everything now.
Flags: needinfo?(gpascutto)
Comment on attachment 8981665 [details] Bug 1434206 - Keep TableUpdate objects in smart pointers. https://reviewboard.mozilla.org/r/247790/#review255852 > dev.platform says we should be using std::move now. Yes and it gets changed in a later patch, though I will probably have to rebase this if they've already removed this from the code.
See Also: → 1467581
Pushed by fmarier@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/56323b585c58 Remove unused and undefined functions. r=gcp https://hg.mozilla.org/integration/autoland/rev/61a5f7d03cef Add const to members and functions that can take it. r=gcp https://hg.mozilla.org/integration/autoland/rev/38f1a6d6877e Clear the current table when protocol parser is done. r=gcp https://hg.mozilla.org/integration/autoland/rev/f2fe038eada3 Use a TableUpdateV2 param in ApplyUpdate(). r=gcp https://hg.mozilla.org/integration/autoland/rev/757ddfe57c0c Don't cache gethash response if we failed to apply results. r=gcp https://hg.mozilla.org/integration/autoland/rev/dbbfc8fd8005 Assert that gethash processing happens on the right thread. r=gcp https://hg.mozilla.org/integration/autoland/rev/00affedf529b Keep TableUpdate objects in smart pointers. r=gcp https://hg.mozilla.org/integration/autoland/rev/653f77b66d23 Make TableUpdate objects const as much as possible. r=gcp https://hg.mozilla.org/integration/autoland/rev/2a73d497179a Replace a pointer with a reference. r=gcp https://hg.mozilla.org/integration/autoland/rev/0980a35455a0 Add const to functions and members that can take it. r=gcp https://hg.mozilla.org/integration/autoland/rev/5d8b0206bfb4 Keep LookupCache objects in smart pointers. r=gcp https://hg.mozilla.org/integration/autoland/rev/080d38cebb99 Make LookupCache objects const as much as possible. r=gcp https://hg.mozilla.org/integration/autoland/rev/789a09ee0980 Clarify when the lookupcache arrays should be cleared. r=gcp https://hg.mozilla.org/integration/autoland/rev/2b8b83d5ce3c Keep CacheResult objects in smart pointers. r=gcp https://hg.mozilla.org/integration/autoland/rev/78b075a48d8f Make DBSserviceWorkerProxy as const as possible. r=gcp https://hg.mozilla.org/integration/autoland/rev/c09d2eeb54af Keep LookupResult objects in smart pointers. r=gcp
Whiteboard: [adv-main62-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: