1120499 - Run ClassifyLocal on the worker thread

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1120145 and https://bugzilla.mozilla.org/show_bug.cgi?id=1120395

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=1f0a4b490a73

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Proxy DoLocalLookup to the worker thread (

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=21e601121a18

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Proxy DoLocalLookup to the worker thread (

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

Try is showing some unified vs. non-unified build brokenness, but nothing should change other than some includes.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

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+.

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=44145e368d07

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=1fa9c97d4e73

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Proxy DoLocalLookup to the worker thread (

Comment 10

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Proxy DoLocalLookup to the worker thread (

Comment 11

[[:mmc] Monica Chew (no longer reading bugmail)]

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

Comment 12

[[:mmc] Monica Chew (no longer reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e00d5f7257b

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e00d5f7257b