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

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

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).

Comment 1

[Tony Chang (Google)]

There's a straightforward fix.  I should have a patch by the end of the day.

Comment 2

[Tony Chang (Google)]

Attachment: 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.

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

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?

Comment 4

[Tony Chang (Google)]

on trunk

Comment 5

[Darin Fisher]

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.

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

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

sr=[e-mail]

Comment 7

[Darin Fisher]

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)

Comment 8

[Tony Chang (Google)]

on branch