Closed Bug 1262339 Opened 4 years ago Closed 6 months ago

Appcache manifest doesn't use URL classifier

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dimi, Assigned: dimi)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: tp-leak)

Attachments

(2 files)

nsManifestCheck::Begin in nsOfflineCacheUpdate.cpp create channel without setting LOAD_CLASSIFY_URI flag
Blocks: 1207775
Assignee: nobody → dlee
Attachment #8738451 - Flags: review?(francois) → review+
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!
Attachment #8738450 - Flags: review?(francois) → review+
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 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 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/
Try server show testcase timeout on windows and mac platform, but no on linux.
I will need to check this again.
Status: NEW → ASSIGNED
(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 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/
Attachment #8738450 - Attachment description: MozReview Request: Bug 1262339 - P1. Appcache manifest doesn't use URL classifier. r?francois → MozReview Request: Bug 1262339 - P1. Appcache manifest doesn't use URL classifier. r=francois
Attachment #8738451 - Attachment description: MozReview Request: Bug 1262339 - P2. Testcase. r=francois → MozReview Request: Bug 1262339 - P2. Classify appcache manifest testcase. r?francois
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/
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...
Flags: needinfo?(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!
Flags: needinfo?(francois)
Keywords: checkin-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
Flags: needinfo?(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
Flags: needinfo?(dlee)
Blocks: 1264169
No longer blocks: 1264169
Depends on: 1264169
Priority: -- → P5
Priority: P5 → P3
not actively working on this right now.
Assignee: dlee → nobody
Status: ASSIGNED → NEW
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?
Flags: needinfo?(francois)
Flags: needinfo?(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.
Flags: needinfo?(dlee)
(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. :)
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.
Flags: needinfo?(francois)
Depends on: 1237782
Whiteboard: tp-leak
Priority: P3 → P2
Assignee: nobody → dlee
Status: NEW → ASSIGNED

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)
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED

Thanks for the extra diligence to verify this!

Depends on: 1584984
You need to log in before you can comment on or make changes to this bug.