Closed Bug 1120499 Opened 5 years ago Closed 5 years ago

Run ClassifyLocal on the worker thread

Categories

(Core :: DOM: Security, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: mmc, Assigned: mmc)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Assignee: nobody → mmc
Status: NEW → ASSIGNED
Attachment #8547859 - Flags: review?(gpascutto)
Attachment #8547859 - Attachment is obsolete: true
Attachment #8547859 - Flags: review?(gpascutto)
Attachment #8547887 - Flags: review?(gpascutto)
Try is showing some unified vs. non-unified build brokenness, but nothing should change other than some includes.
Comment on attachment 8547887 [details] [diff] [review]
Proxy DoLocalLookup to the worker thread (

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

LGTM but please do a followup to get rid of the extra mCryptoHash if feasible.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1356,5 @@
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>  
> +  // In unittests, we may not have been initalized, so don't crash.
> +  rv = mWorkerProxy->DoLocalLookup(key, tables, nullptr, results);

The whole "make an extra mCryptoHash for the main thread" change might now be superfluous? Let's get rid of it if that's the case. If it needs to say for some other reason, r+.
Attachment #8547887 - Flags: review?(gpascutto) → review+
Attachment #8547887 - Attachment is obsolete: true
Attachment #8548541 - Attachment is obsolete: true
Comment on attachment 8548542 [details] [diff] [review]
Proxy DoLocalLookup to the worker thread (

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

Removed extraneous nsCryptoHash stuff and also fixed all my unified v. non-unified errors. There ended up being quite a lot of changes so you might want to re-review.

::: toolkit/components/url-classifier/nsUrlClassifierProxies.cpp
@@ +138,5 @@
> +  nsIThread* t = nsUrlClassifierDBService::BackgroundThread();
> +  if (!t)
> +    return NS_ERROR_FAILURE;
> +
> +  mozilla::SyncRunnable::DispatchToThread(t, r);

khuey told me I was doing this wrong before -- NS_DISPATCH_SYNC was causing crashes due to DoLocalLookup not returning before JS was operating on the channel. But now try is looking pretty green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf8549a27692
Attachment #8548542 - Flags: review?(gpascutto)
Attachment #8548542 - Flags: review?(gpascutto) → review+
https://hg.mozilla.org/mozilla-central/rev/0e00d5f7257b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1122691
You need to log in before you can comment on or make changes to this bug.