ordering bug causes some urls to not be inspected on new windows

RESOLVED FIXED in Firefox 2 beta1

Status

()

Toolkit
Safe Browsing
RESOLVED FIXED
12 years ago
4 years ago

People

(Reporter: Tony Chang (Google), Assigned: Tony Chang (Google))

Tracking

({fixed1.8.1})

Trunk
Firefox 2 beta1
fixed1.8.1
Points:
---
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: swag:1day)

Attachments

(1 attachment)

(Assignee)

Description

12 years ago
To avoid Ts regressions, the JS code for safebrowsing is loaded a couple seconds after Chrome window load.  This could cause some URLs (like the URL opening the browser window) to be missed, so once safebrowsing code loads, we check all the tabs and iframes for phishing urls.

Unfortunately, this misses urls that have already started, but data hasn't begun transfering (url isn't set until the request fires ondataavailable).
(Assignee)

Comment 1

12 years ago
There's a straightforward fix.  I should have a patch by the end of the day.
Status: NEW → ASSIGNED
Flags: blocking-firefox2?
Whiteboard: swag:1day
Target Milestone: --- → Firefox 2 beta1
(Assignee)

Comment 2

12 years ago
Created attachment 226430 [details] [diff] [review]
v1: setup the webprogresslistener immediately (before js load)

Setup the webprogresslistener immediately and just collect requests/urls in an array.  Once the js code loads, check the requests in the array.
Attachment #226430 - Flags: review?(mmchew)
Attachment #226430 - Flags: approval1.8.1?
Comment on attachment 226430 [details] [diff] [review]
v1: setup the webprogresslistener immediately (before js load)

Looks fine.

>Index: browser/components/safebrowsing/content/sb-loader.js
>===================================================================
>--- browser/components/safebrowsing/content/sb-loader.js	old
>+++ browser/components/safebrowsing/content/sb-loader.js	new
>@@ -39,33 +39,48 @@
>  * This file is included into the main browser chrome from
>  * browser/base/content/global-scripts.inc
>  */
> 
> var safebrowsing = {
>   controller: null,
>   phishWarden: null,
> 
>+  // We set up the web progress listener immediately so we don't miss any
>+  // phishing urls.  Since the phishing infrastructure isn't loaded yet, we
>+  // just store the urls in a list.
>+  progressListener: null,
>+  progressListenerCallback: {
>+    requests: [],
>+    onDocNavStart: function(request, url) {
>+      this.requests.push({
>+        'request': request,
>+        'url': url
>+      });
>+    }
>+  },
Check requests is non-null here in case of spurious onDocNavStart events that get triggered after deferred start cleanup?
Attachment #226430 - Flags: review?(mmchew) → review+
(Assignee)

Comment 4

12 years ago
on trunk

Comment 5

12 years ago
Comment on attachment 226430 [details] [diff] [review]
v1: setup the webprogresslistener immediately (before js load)

browser/ changes require at least a browser peer or owner to review the changes.  Over to mconnor, but you could also try to get review from ben.

Please re-request branch approval once you have sr from a browser/ reviewer.
Attachment #226430 - Flags: approval1.8.1? → superreview?(mconnor)
Comment on attachment 226430 [details] [diff] [review]
v1: setup the webprogresslistener immediately (before js load)

sr=ben@mozilla.org
Attachment #226430 - Flags: superreview?(mconnor) → superreview+
(Assignee)

Updated

12 years ago
Attachment #226430 - Flags: approval1.8.1?

Comment 7

12 years ago
Comment on attachment 226430 [details] [diff] [review]
v1: setup the webprogresslistener immediately (before js load)

a=darin on behalf of drivers (please land on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to the bug)
Attachment #226430 - Flags: approval1.8.1? → approval1.8.1+

Updated

12 years ago
Flags: blocking-firefox2? → blocking-firefox2+
(Assignee)

Comment 8

12 years ago
on branch
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Component: Phishing Protection → Phishing Protection
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.