Closed Bug 1434206 Opened 5 years ago Closed 5 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: 5 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.