1262406 - Track element doesn't use the URL classifier

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Dimi Lee [ooo 16.04 - 21.04］]

HTML Track element open a channel when load resource, but it doesn't specify the nsIChannel::LOAD_CLASSIFY_URI loadflag when open channel

Comment 1

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: WIP patch

Comment 2

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: MozReview Request: Bug 1262406 - P1. Track element doesn't use the URL classifier. r=francois

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

Comment 3

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: MozReview Request: Bug 1262406 - P2. Classify track element testcase. r=francois

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

Comment 4

[François Marier [:francois]]

Comment on attachment 8738932 [details]
MozReview Request: Bug 1262406 - P1. Track element doesn't use the URL classifier. r=francois

https://reviewboard.mozilla.org/r/44859/#review41565

::: dom/html/HTMLTrackElement.cpp:218
(Diff revision 1)
>                       uri,
>                       static_cast<Element*>(this),
>                       nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS,
>                       nsIContentPolicy::TYPE_INTERNAL_TRACK,
> -                     loadGroup);
> +                     loadGroup,
> +                     nullptr,

Nit: personally, I like to leave a comment next to the `nullptr` here just to label it and help show it was intentional:
https://hg.mozilla.org/mozilla-central/rev/cc28ecb2b687#l1.21

Comment 5

[François Marier [:francois]]

Comment on attachment 8738933 [details]
MozReview Request: Bug 1262406 - P2. Classify track element testcase. r=francois

https://reviewboard.mozilla.org/r/44861/#review41567

::: toolkit/components/url-classifier/tests/mochitest/test_classify_track.html:21
(Diff revision 1)
> +  const PREF = "browser.safebrowsing.malware.enabled";
> +  const track_path = "tests/toolkit/components/url-classifier/tests/mochitest/basic.vtt";
> +
> +  // Use different url to avoid url classifier bypass classify because of cache.
> +  const malware_url1 = "test1.example.org/";
> +  const malware_url2 = "test2.example.org/";

I wonder here if you could simply add a bogus query string parameter to turn the second one into a different URL for cache purposes:

example.org/tests/toolkit/components/url-classifier/tests/mochitest/basic.vtt

and

example.org/tests/toolkit/components/url-classifier/tests/mochitest/basic.vtt?againplease

I'm being a bit paranoid here, but the advantage of doing it this way is that you can add a single entry to the malware list and then you're actually checking the same blacklisted domain in the last two tests (testBlacklistTrackSafebrowsingOff and testBlacklistTrackSafebrowsingOn).

The issue with tests is that there's nothing to "test the tests", so I like to keep them as simple as possible to make sure they don't start to silently pass when they shouldn't :)

Comment 6

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to François Marier [:francois] from comment #5)
> Comment on attachment 8738933 [details]
> MozReview Request: Bug 1262406 - P2. Classify track element testcase.
> I wonder here if you could simply add a bogus query string parameter to turn
> the second one into a different URL for cache purposes:
> 
> example.org/tests/toolkit/components/url-classifier/tests/mochitest/basic.vtt
> 
> and
> 
> example.org/tests/toolkit/components/url-classifier/tests/mochitest/basic.
> vtt?againplease

Yes, this can avoid cache! Thanks for the suggestion :)

Comment 7

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8738932 [details]
MozReview Request: Bug 1262406 - P1. Track element doesn't use the URL classifier. r=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44859/diff/1-2/

Comment 8

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8738933 [details]
MozReview Request: Bug 1262406 - P2. Classify track element testcase. r=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44861/diff/1-2/

Comment 9

[François Marier [:francois]]

Comment on attachment 8738933 [details]
MozReview Request: Bug 1262406 - P2. Classify track element testcase. r=francois

https://reviewboard.mozilla.org/r/44861/#review41843

Comment 10

[François Marier [:francois]]

https://reviewboard.mozilla.org/r/44859/#review41845

::: dom/html/HTMLTrackElement.cpp:217
(Diff revision 2)
>    rv = NS_NewChannel(getter_AddRefs(channel),
>                       uri,
>                       static_cast<Element*>(this),
>                       nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS,
>                       nsIContentPolicy::TYPE_INTERNAL_TRACK,
> -                     loadGroup);
> +                     loadGroup, // aLoadGroup

nit: that one is pretty obvious from the name of the variable already :)

Comment 11

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8738932 [details]
MozReview Request: Bug 1262406 - P1. Track element doesn't use the URL classifier. r=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44859/diff/2-3/

Comment 12

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8738933 [details]
MozReview Request: Bug 1262406 - P2. Classify track element testcase. r=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44861/diff/2-3/

Comment 13

[Dimi Lee [ooo 16.04 - 21.04］]

(In reply to Dimi Lee[:dimi][:dlee] from comment #11)
> Comment on attachment 8738932 [details]
> MozReview Request: Bug 1262406 - P1. Track element doesn't use the URL
> classifier. r=francois
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/44859/diff/2-3/

Address nit.

Comment 14

[Dimi Lee [ooo 16.04 - 21.04］]

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

Comment 15

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8738932 [details]
MozReview Request: Bug 1262406 - P1. Track element doesn't use the URL classifier. r=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44859/diff/3-4/

Comment 16

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8738933 [details]
MozReview Request: Bug 1262406 - P2. Classify track element testcase. r=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44861/diff/3-4/

Comment 17

[Dimi Lee [ooo 16.04 - 21.04］]

try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f19fd7ed117e

Comment 18

[Dimi Lee [ooo 16.04 - 21.04］]

remove checkin-needed because this patch has similar problem as bug 1264169, also we should land any urlclassifier testcase after bug 1264169 is fixed

Comment 19

[Dimi Lee [ooo 16.04 - 21.04］]

try result after rebasing to latest code and apply changes in bug 1264168
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fcd8bd4b413

Comment 20

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8738932 [details]
MozReview Request: Bug 1262406 - P1. Track element doesn't use the URL classifier. r=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44859/diff/4-5/

Comment 21

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8738933 [details]
MozReview Request: Bug 1262406 - P2. Classify track element testcase. r=francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44861/diff/4-5/

Comment 22

[Dimi Lee [ooo 16.04 - 21.04］]

try link is in Comment 19

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/aea79c4f38f4
https://hg.mozilla.org/integration/fx-team/rev/77c1d14c1e1f

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aea79c4f38f4
https://hg.mozilla.org/mozilla-central/rev/77c1d14c1e1f