1262339 - Appcache manifest doesn't use URL classifier

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[Dimi Lee [:dimi][:dlee]]

nsManifestCheck::Begin in nsOfflineCacheUpdate.cpp create channel without setting LOAD_CLASSIFY_URI flag

Comment 1

[Dimi Lee [:dimi][:dlee]]

Attachment: MozReview Request: Bug 1262339 - P1. Appcache manifest doesn't use URL classifier. r=francois

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

Comment 2

[Dimi Lee [:dimi][:dlee]]

Attachment: MozReview Request: Bug 1262339 - P2. Classify appcache manifest testcase. r?francois

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

Comment 3

[François Marier [:francois]]

Comment on attachment 8738451 [details]
MozReview Request: Bug 1262339 - P2. Classify appcache manifest testcase. r?francois

https://reviewboard.mozilla.org/r/44553/#review41399

The test looks great, thanks!

Comment 4

[François Marier [:francois]]

Comment on attachment 8738450 [details]
MozReview Request: Bug 1262339 - P1. Appcache manifest doesn't use URL classifier. r=francois

https://reviewboard.mozilla.org/r/44551/#review41401

Comment 5

[Dimi Lee [:dimi][:dlee]]

Comment on attachment 8738450 [details]
MozReview Request: Bug 1262339 - P1. Appcache manifest doesn't use URL classifier. r=francois

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

Comment 6

[Dimi Lee [:dimi][:dlee]]

Comment on attachment 8738451 [details]
MozReview Request: Bug 1262339 - P2. Classify appcache manifest testcase. r?francois

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

Comment 7

[Dimi Lee [:dimi][:dlee]]

Try server show testcase timeout on windows and mac platform, but no on linux.
I will need to check this again.

Comment 8

[Dimi Lee [:dimi][:dlee]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #7)
> Try server show testcase timeout on windows and mac platform, but no on
> linux.
> I will need to check this again.

ah...It turns out to be my fault.
The testcase send XMLHttpRequest to sjs to update server state(ensure cache manifest is updated for each testcase). But the url of sjs is mochi.test which is already added to malware database, so the XMLHttpRequest will be blocked.
The reason that some platform seems work is because of timing, not actually related to platform. It has the same root cause as Bug1233914 comment 8.

Comment 9

[Dimi Lee [:dimi][:dlee]]

Comment on attachment 8738450 [details]
MozReview Request: Bug 1262339 - P1. Appcache manifest doesn't use URL classifier. r=francois

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

Comment 10

[Dimi Lee [:dimi][:dlee]]

Comment on attachment 8738451 [details]
MozReview Request: Bug 1262339 - P2. Classify appcache manifest testcase. r?francois

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

Comment 11

[Dimi Lee [:dimi][:dlee]]

Hi François,
Based on Comment 7 & Comment 8, I updated patch P2, could you help review again ? Thanks.
I don't know how to force r? again with mozreview, so use ni here...

Comment 12

[François Marier [:francois]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #11)
> Based on Comment 7 & Comment 8, I updated patch P2, could you help review
> again ? Thanks.

Looks good to me. Thanks!

Comment 13

[Dimi Lee [:dimi][:dlee]]

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

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/fe000d2b968f
https://hg.mozilla.org/integration/fx-team/rev/111b90ed7c50

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out in https://hg.mozilla.org/integration/fx-team/rev/4cf4428ef394 for a persistent leak:
https://treeherder.mozilla.org/logviewer.html#?job_id=8603490&repo=fx-team

I also saw this other test failure a few times that seemed related to these patches:
https://treeherder.mozilla.org/logviewer.html#?job_id=8594156&repo=fx-team

Comment 16

[Dimi Lee [:dimi][:dlee]]

(In reply to Wes Kocher (:KWierso) from comment #15)
> I had to back these out in
> https://hg.mozilla.org/integration/fx-team/rev/4cf4428ef394 for a persistent
> leak:
> https://treeherder.mozilla.org/logviewer.html#?job_id=8603490&repo=fx-team
> 
> I also saw this other test failure a few times that seemed related to these
> patches:
> https://treeherder.mozilla.org/logviewer.html#?job_id=8594156&repo=fx-team

oh sorry I didn't notice that when checking try :(
The test failure is related to not clean malware database properly.
For the persistent leak I will still need some time to look at it

Comment 17

[Dimi Lee [:dimi][:dlee]]

not actively working on this right now.

Comment 18

[Ethan Tseng [:ethan]]

Correct me if I am wrong.

https://developer.mozilla.org/en-US/docs/Web/HTML/Using_the_application_cache
According to this document, AppCache has been removed from the Web standards.
I suggest we should simply close this bug, shouldn't we?

Comment 19

[Dimi Lee [:dimi][:dlee]]

We can make this very low priority, P5, for example.
Since the APP cache code path is not yet removed and still supported by firefox, I think we can leave it open for now.

Comment 20

[Ethan Tseng [:ethan]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #19)
> We can make this very low priority, P5, for example.
> Since the APP cache code path is not yet removed and still supported by
> firefox, I think we can leave it open for now.

Agree.
I'll leave it to Francois to decide the priority. :)

Comment 21

[François Marier [:francois]]

I agree it should be a lower priority than the other callsites.

However, until it gets removed from mozilla-central, we still need to do it. Since P5 is "wontfix unless someone contributes a patch", I think we should leave it in the backlog (P3). It's not like we'll be addressing everything in the backlog anytime soon.

Comment 22

[Dimi Lee [:dimi][:dlee]]

I have verified that this has been fixed by Bug 1522412 by:

1. visit http://webdbg.com/test/appcache/ (enable preference "browser.cache.offline.insecure.enable")
   check about:cache to make sure "http://webdbg.com/test/appcache/manifest.aspx" is in the cache
2. clear all the cache
3. Add "http://webdbg.com/test/appcache/manifest.aspx" to safe browsing malware database
4. visit http://webdbg.com/test/appcache/
   and check about:cache to make sure "http://webdbg.com/test/appcache/manifest.aspx" is NOT in the cache.
   (also double confirmed by checking safe browsing log)

Comment 23

[Ethan Tseng [:ethan]]

Thanks for the extra diligence to verify this!