1291024 - Intermittent toolkit/components/url-classifier/tests/mochitest/test_gethash.html | Should not import bad css - didn't expect "hidden", but got it

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/logviewer.html#?job_id=3174443&repo=mozilla-aurora#L2496

Comment 2

[Intermittent Failures Robot]

7 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 3
* try: 1
* mozilla-aurora: 1
* fx-team: 1
* autoland: 1

Platform breakdown:
* android-4-3-armv7-api15: 6
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1291024&startday=2016-08-01&endday=2016-08-07&tree=all

Comment 3

[Ethan Tseng [:ethan]]

Dimi, is this bug related to bug 1289028?
Are we working on this?

Comment 4

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Ethan Tseng [:ethan] from comment #3)
> Dimi, is this bug related to bug 1289028?
> Are we working on this?

No, that's different.
I will check this one.

Comment 5

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Patch

Some testcase share same html file including the resources html load.
Caching cause load bypass classifier check

Comment 6

[Henry Chang [:hchang]]

Comment on attachment 8783463 [details] [diff] [review]
Patch

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

Everything makes sense to me :) I only wonder if the network predictor is potentially conflicting to safe browsing.

Thanks!

::: toolkit/components/url-classifier/tests/mochitest/test_gethash.html
@@ +141,5 @@
>  
>  SimpleTest.waitForExplicitFinish();
> +
> +// 'network.predictor.enabled' is disabled because it may cause
> +// we load cached resouce(evil.js, evil.css, bad.css) and bypass classifier check

Does it mean the network predictor feature is unsafe?

Comment 7

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Henry Chang [:henry][:hchang] from comment #6)
> Comment on attachment 8783463 [details] [diff] [review]
> Patch
> 
> Review of attachment 8783463 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Everything makes sense to me :) I only wonder if the network predictor is
> potentially conflicting to safe browsing.
> 
> Thanks!
> 
> ::: toolkit/components/url-classifier/tests/mochitest/test_gethash.html
> @@ +141,5 @@
> >  
> >  SimpleTest.waitForExplicitFinish();
> > +
> > +// 'network.predictor.enabled' is disabled because it may cause
> > +// we load cached resouce(evil.js, evil.css, bad.css) and bypass classifier check
> 
> Does it mean the network predictor feature is unsafe?

Not exactly, in this case both cache and network predictor requires successful connection to resource.
If we always block the malware url, then network predictor won't take affect.

The evil.js ...etc will be loaded because in the following testcase we want to test gethash failure.
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/tests/mochitest/test_gethash.html#131

Comment 8

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Bug 1291024 - Avoid caching safebrowsing testcase resources.

Review commit: https://reviewboard.mozilla.org/r/73520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73520/

Comment 9

[François Marier [:francois]]

Comment on attachment 8783852 [details]
Bug 1291024 - Avoid caching safebrowsing testcase resources.

https://reviewboard.mozilla.org/r/73520/#review71578

Comment 10

[Dimi Lee [ooo 16.04 - 21.04］]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19435a391a4b

Comment 11

[Dimi Lee [ooo 16.04 - 21.04］]

try:
https://hg.mozilla.org/try/rev/77b3b66621ebfc68789e27970957c4f2a3157061

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c857c69bd739
Avoid caching safebrowsing testcase resources. r=francois

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c857c69bd739

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b6d35508be5b