Support SafeBrowsing in b2g

RESOLVED FIXED in Firefox 38

Status

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: fabrice, Assigned: yu-yamamoto)

Tracking

({sec-want})

unspecified
2.2 S6 (20feb)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed, b2g-v2.2 affected, b2g-master affected)

Details

(Whiteboard: [adv-main38-])

Attachments

(5 attachments, 10 obsolete attachments)

22.78 KB, patch
gcp
: review+
Details | Diff | Splinter Review
22.73 KB, patch
gcp
: review+
Details | Diff | Splinter Review
2.31 MB, text/plain
Details
2.11 MB, text/plain
Details
22.44 KB, patch
Details | Diff | Splinter Review
Reporter

Description

7 years ago
We need that for general well being of our users using the browser app, as well as for blocking rogue apps.
Can you please prvoide more information or documents for softBrowsing?

Comment 2

6 years ago
Any update here?

Thre is Safebrowing implemented in b2g but doesn't look work.
Any plan to embed a func which "Firefox browser" already has and when?
gcp, is this something that KDDI could help contribute to?
Flags: needinfo?(gpascutto)
I have no idea who KDDI is, but anyone with JavaScript skills and that knows how to debug on B2G can start looking if the SafeBrowsing js files/service is being loaded, and if they are, if something there diverges from what happens on desktop/Android.

There's little particular reasons why B2G SafeBrowsing should have much if any difference from the desktop one, I'd think, given that the backendsource is platform-agnostic C++ and a bunch of JavaScript for the services and UI. So checking if there's any differences in the sources right now, and figuring out if those are intended, would be a good starting point.
Flags: needinfo?(gpascutto)
Getting safebrowsing to work in a multiprocess world is probably a large portion of the work here.

IIRC safebrowsing needs 1mb or so of data, so we don't want to load this into every child process.  Child processes would have to proxy their requests to the parent.

gcp says he doesn't know if this code exists at the moment.
The backend that handles the lookups already lives in a seperate thread with a queue protected with mutexes. So I don't think that is the problem.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #6)
> The backend that handles the lookups already lives in a seperate thread with
> a queue protected with mutexes. So I don't think that is the problem.

Perhaps we're not talking about the same thing.

When you get an XPCOM service in a child process, it does not magically proxy calls to a corresponding service in the parent, regardless of whether the service in the parent is threadsafe.
>Can you please prvoide more information or documents for softBrowsing?

https://developers.google.com/safe-browsing/developers_guide_v2
>Perhaps we're not talking about the same thing.

I was referring to this:

"IIRC safebrowsing needs 1mb or so of data, so we don't want to load this into every child process."

This is already split off, thread-wise, from the parent thread that is the XPCOM service.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #9)
> I was referring to this:
> 
> "IIRC safebrowsing needs 1mb or so of data, so we don't want to load this
> into every child process."
> 
> This is already split off, thread-wise, from the parent thread that is the
> XPCOM service.

Sorry, I still don't understand what that has to do with whether or not we load the data into a child process.

B2G has a multiprocess architecture.  At the risk of being pedantic: Processes get private address spaces.  This means that the data in the main process which the safebrowsing thread uses is in a different address space from all of the threads in all of the child processes, and is therefore inaccessible to the child processes, unless we explicitly share it.
Currently it looks like this:

-> Main FF Thread -> SafeBrowsing URLfoo service <=> queue ^ 
  -> SafeBrowsing background/IO thread           <=> queue v

Assuming B2G has something of a parent process:

-> Parent FF Process
  -> SafeBrowsing background/IO thread <=========================>
  -> Child FF Process -> SafeBrowsing URLfoo service <=> |       ^
  -> Child FF Process -> SafeBrowsing URLfoo service <=> | queue v
  -> Child FF Process -> SafeBrowsing URLfoo service <=> |

The background/IO thread is the one that contains the data. Basically, you wouldn't ever want to have two of the IO threads anyway (it would fail spectacularly on updates and even make things slower anyway due to disk contention), so having 2 copies of the data is not really an issue - if you get there you're already totally screwed anyway. That part of the code also has some infrastructure for communicating between threads. You have to move the current queue to shared memory if you go from threads to processes, but once you do that you can do the lookups from any process. (I'm going to sortof assume we already have an interprocess-queue thingie in our tree?)

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#186

What I'm trying to say is that the point where you need to communicate and share data between the processes is fairly clear, at least if we're talking about the "don't duplicate the database" problem.

Now, as for everything else, that's another matter.

Those JavaScript callbacks in the queue might end up being more interesting. Also, who handles the updates? Currently they're launched from JS in the main thread, and proxied over:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierProxies.cpp 
Can this go in the "Parent FF Process" in my diagram?
Okay, I think we understand each other.  Thanks.

> Also, who handles the updates? Currently they're launched from JS in the main thread, and proxied over:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierProxies.cpp 
> Can this go in the "Parent FF Process" in my diagram?

I think so.
Duplicate of this bug: 826808
SafeBrowsing is a security check, and network security checks should be done by the parent process anyway, not by the child process. So, there should be no SafeBrowsing code running in the child process in the first place. IIRC, the SafeBrowsing service suspects the channel while the check is done, and then resumes it when the check is complete. That suspend/resume should happen in the parent process. So, there shouldn't be any need for complicated proxying code. Instead, I think the difficulty is in migrating the code that connects the safe browsing from the place it is currently done (which would run in the child processes) to a place that runs in the parent processes. If things are done this way, then a compromised child process will not be able to influence the SafeBrowsing decision.

Also note that, as of now, NSS and most of PSM are unavailable in child processes, and URL classifier does use NSS/PSM for its cryptography. And, certificate validation and other network security checks are done the same way.
Assignee

Comment 15

5 years ago
Posted patch [WIP]safebrowsing.patch (obsolete) — Splinter Review
Hi Blake,

I’ve been coded safe browsing on firefox OS.
This is a work in progress patch for safe browsing on firefox OS with gecko part.
Actually, the patch has some issues for contribute.
I’d like to contribute safe browsing feature for firefox OS, although, I’ve been facing the issues...

May I have a suggestion about the bellow issues?

1. Is this code suitable for the design of gecko.
   That mean, it will be happy for me to get some comment to my WIP patch.
2. jslib doesn’t work correctly.
   in my code, the object of jslib has “FakeBackStagePath”, then it can’t access some modules[1] that over jslib.
   [1] : the modules under “ B2G/gecko/toolkit/components/url-classifier/content “, e.g. jslib.RequestBackoff, jslib.Function.prototype.inherits

Ragards,
Yusuke.
Flags: needinfo?(mrbkap)
Comment on attachment 8502201 [details] [diff] [review]
[WIP]safebrowsing.patch

Review of attachment 8502201 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/SafeBrowsing.jsm
@@ +127,4 @@
>        var clientID = Services.appinfo.name;
>      }
>  
> +    clientID = "Firefox";

You need to get this from the prefs instead of overwriting it incorrectly for all platforms. Preferably so that official builds get the expected string and the others just report something like Aurora/Nightly etc. See the comment in b2g.js.

::: toolkit/components/url-classifier/content/moz/alarm.js
@@ +129,4 @@
>    this.debugZone = "conditionalalarm";
>  }
>  
> +//G_ConditionalAlarm.inherits(G_Alarm);

Why are you changing this code?

::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
@@ +158,5 @@
>        // Initialize request backoffs separately, since requests are deleted
>        // after they are dispatched.
> +      //var jslib = Cc["@mozilla.org/url-classifier/jslib;1"]
> +                  //.getService().wrappedJSObject;
> +      //this._backoffs[aGethashUrl] = new jslib.RequestBackoff(

Need to get this fixed.
Attachment #8502201 - Flags: review-
Assignee

Comment 17

5 years ago
Hi gcp,

Thank you for your review, 

(In reply to Gian-Carlo Pascutto [:gcp] from comment #16)
> ::: toolkit/components/url-classifier/content/moz/alarm.js
> @@ +129,4 @@
> >    this.debugZone = "conditionalalarm";
> >  }
> >  
> > +//G_ConditionalAlarm.inherits(G_Alarm);
> 
> Why are you changing this code?
> 
> ::: toolkit/components/url-classifier/nsUrlClassifierHashCompleter.js
> @@ +158,5 @@
> >        // Initialize request backoffs separately, since requests are deleted
> >        // after they are dispatched.
> > +      //var jslib = Cc["@mozilla.org/url-classifier/jslib;1"]
> > +                  //.getService().wrappedJSObject;
> > +      //this._backoffs[aGethashUrl] = new jslib.RequestBackoff(
> 
> Need to get this fixed.
That code can't load jslib( see my comment #15 issue.2 ); because, I coded that. 
Actuary, I did't have no idea to fix to load jslib on that code.
If you wouldn't mind, would you be able to suggest me to how to fix it?
(In reply to Yusuke Yamamoto from comment #17)

> Actuary, I did't have no idea to fix to load jslib on that code.
> If you wouldn't mind, would you be able to suggest me to how to fix it?

I don't know anything about this, sorry.
Assignee

Comment 19

5 years ago
(In reply to Gian-Carlo Pascutto [:gcp] from comment #18)
> (In reply to Yusuke Yamamoto from comment #17)
> 
> > Actuary, I did't have no idea to fix to load jslib on that code.
> > If you wouldn't mind, would you be able to suggest me to how to fix it?
> 
> I don't know anything about this, sorry.
I got it, thanks.
In Firefox Browser, that source file can load jslib.
Although, in Firefox OS, the source files can not load jslib( comment #15 issue.2 ).
That's why I coded comment out and modify the sources.
I wonder why the sources can not load jslib, the same code as firefox browser and firefox os....

I'm trying to continue to fix that...
Assignee

Comment 20

5 years ago
In nsUrlClassifierListManager.js at "Firefox OS" code, it can't be loaded( refer ) to jslib( comment #15 issue.2 ).
Then, I have tried to log jslib object.

> function Init() {
>   // Pull the library in.
>   var jslib = Cc["@mozilla.org/url-classifier/jslib;1"]
>               .getService().wrappedJSObject;
The log said , [object FakeBackStagePass].

Although, nsUrlClassifierListManager.js at "Firefox Browser" code, the log said , [object BackStagePass].

I was wondering if why jslib object has diffent object.
# In Firefox OS     : FakeBackStagePass.
# In Firfox Browser : BackStagePass.

It seems like that there is a compile option to enalbe / disabel jslib object, because Firefxo OS has "Fake" obeject.

I would like to have any suggestion about this issue....
Assignee

Comment 21

5 years ago
Hi, gcp

I've just finished one issue ( comment #15 issue.2 ) on my code.
This attachment patch is that.

Although, that patch still have another bellow issue that you pointed out.

> ::: toolkit/components/url-classifier/SafeBrowsing.jsm
> @@ +127,4 @@
> >        var clientID = Services.appinfo.name;
> >      }
> >  
> > +    clientID = "Firefox";
> You need to get this from the prefs instead of overwriting it incorrectly for all > platforms. Preferably so that official builds get the expected string and the 
> others just report something like Aurora/Nightly etc. See the comment in b2g.js.

I'd like to have any suggestion about this clientID.
In safebrowsing services with google server does not allow with "B2G" client ID.
Then, clientID was replaced with "Firefox".
I'd like to add new prefs like "Services.appinfo.name", but I couldn't judge that to add new prefs ( e.g. "Service.b2g.safebrowsing.clientID" ) will be OK or not.

Could I get your some comment?
Flags: needinfo?(gpascutto)
The code that you are overruling with that line was exactly there to read this as a pref, i.e. browser.safebrowsing.id. So put that pref to "Firefox" in B2G's config.
Flags: needinfo?(gpascutto)
Reporter

Comment 23

5 years ago
Hi Yusuke, do you have time to finish that patch or do you need help? I'd like us to land that soon if possible. Thanks!
Flags: needinfo?(yu-yamamoto)
Assignee

Comment 24

5 years ago
Hi Fabrice,

I'll have coded to finish (with gcp comment #22) this patch until next week.

Although, actually, there are some things to discuss to complete this patch.
I'm wondering why safebrowsing feature hadn't supported on B2G?
I mean, there might be any performance concern...? , i.e. RAM resources, CPU resources, data traffic...

Did you know that why B2G does not support safebrowsing feature?

Firefox OS will be support various phone spec, so, the user who uses low spec model, it might not be used safebrowsing feature.

Then, I think, do we have to measure the performance with safebrowsing on b2g?
Flags: needinfo?(yu-yamamoto) → needinfo?(fabrice)
Reporter

Comment 25

5 years ago
(In reply to Yusuke Yamamoto from comment #24)
> Hi Fabrice,
> 
> I'll have coded to finish (with gcp comment #22) this patch until next week.

Ok, perfect!

> Although, actually, there are some things to discuss to complete this patch.
> I'm wondering why safebrowsing feature hadn't supported on B2G?
> I mean, there might be any performance concern...? , i.e. RAM resources, CPU
> resources, data traffic...

We had concerns about memory usage and general performance, yes. Also, it was quite low priority and no-one had time to work on it before you.

> Firefox OS will be support various phone spec, so, the user who uses low
> spec model, it might not be used safebrowsing feature.

Yes, it would be good to have a pref to disable/enable easily this feature.

> Then, I think, do we have to measure the performance with safebrowsing on
> b2g?

That would be good yes, namely the memory usage, and checking that we don't regress app startup time.
Flags: needinfo?(fabrice)
The requirement is about 2M of RAM permanently, about 2M extra on updates (every half hour) and about 3M of storage (temporarily another 3M on updates). Browsing overhead should be minimal, it's <1ms (lowest telemetry bin) on desktop.

Bandwidth is documented in bug 731525.

Can be easily enabled/disabled via prefs.
Assignee

Comment 27

5 years ago
Posted patch 20141119_safebrowsing.patch (obsolete) — Splinter Review
(In reply to Fabrice Desré [:fabrice] from comment #25)
> > I'll have coded to finish (with gcp comment #22) this patch until next week.
> 
> Ok, perfect!
It had attached on this record.

> Yes, it would be good to have a pref to disable/enable easily this feature.
OK, I would say that could you file a bug?, and I think it will have to get UI spec.



> That would be good yes, namely the memory usage, and checking that we don't
> regress app startup time.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #26)
> The requirement is about 2M of RAM permanently, about 2M extra on updates
> (every half hour) and about 3M of storage (temporarily another 3M on
> updates). Browsing overhead should be minimal, it's <1ms (lowest telemetry
> bin) on desktop.
OK, I got it with the viewpoint of memory usage, startup time.

But, I'm not familiar with that how to measure that, what kind of point, on Firefox OS. 
If we have to measure it, I think that it will be need to clear up( or summarize ) that.


Although, it will be happy to review my patch.
Attachment #8525123 - Flags: review?(gpascutto)
Attachment #8525123 - Flags: review?(fabrice)
Comment on attachment 8525123 [details] [diff] [review]
20141119_safebrowsing.patch

Review of attachment 8525123 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/app/b2g.js
@@ +358,5 @@
> +pref("browser.safebrowsing.reportPhishURL", "http://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%");
> +pref("browser.safebrowsing.reportMalwareURL", "http://%LOCALE%.malware-report.mozilla.com/?hl=%LOCALE%");
> +pref("browser.safebrowsing.reportMalwareErrorURL", "http://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%");
> +pref("browser.safebrowsing.appRepURL", "https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%");
> +pref("browser.safebrowsing.b2g.clientId", "Firefox");

Why is this needed? Why not simply set the existing pref (see line below this).

@@ +367,5 @@
> +pref("browser.safebrowsing.id", "navclient-auto-ffox");
> +#endif
> +
> +// Tables for application reputation.
> +pref("urlclassifier.downloadBlockTable", "goog-badbinurl-shavar");

Does the B2G browser actually have a download system that can use these?

::: toolkit/components/url-classifier/SafeBrowsing.jsm
@@ +130,5 @@
> +    if (clientID == "B2G") {
> +      try {
> +        clientID = Services.prefs.getCharPref("browser.safebrowsing.b2g.clientId");
> +      } catch(e) {}
> +    }

Again, not sure why B2G needs a separate pref if a pref already exists for this.

::: toolkit/components/url-classifier/content/listmanager.js
@@ +16,4 @@
>  //      that the listmanagers tables are properly written on updates
>  
>  // Log only if browser.safebrowsing.debug is true
> +this.log = function log(...stuff) {

Why are these changes needed?

::: toolkit/components/url-classifier/content/moz/alarm.js
@@ +130,5 @@
>    G_Alarm.call(this, callback, delayMS, opt_repeating, opt_maxTimes);
>    this.debugZone = "conditionalalarm";
>  }
>  
> +G_ConditionalAlarm.inherits = function(parentCtor) {

Same question. Why does .inherits suddenly have to be defined? What makes B2G so special here?

I'm not an expert in JavaScript but requiring changes to these files which are literally years old for no direct reason that I can see makes me very, very nervous.
Attachment #8525123 - Flags: review?(gpascutto) → review-
Monica, do you know if B2G has the hooks in place for the download scanner?
Flags: needinfo?(mmc)
Reporter

Comment 30

5 years ago
gcp, the js changes are due to b2g using a shared global (http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#839).

I agree that we should not need new prefs for b2g.
Thanks, I researched this a bit and the following is obviously very relevant:
https://bugzilla.mozilla.org/show_bug.cgi?id=989373#c17
Reporter

Updated

5 years ago
Attachment #8525123 - Flags: review?(fabrice)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #29)
> Monica, do you know if B2G has the hooks in place for the download scanner?

Paolo, does b2g use the new downloads API?
Flags: needinfo?(mmc) → needinfo?(paolo.mozmail)
Reporter

Comment 33

5 years ago
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #32)
> (In reply to Gian-Carlo Pascutto [:gcp] from comment #29)
> > Monica, do you know if B2G has the hooks in place for the download scanner?
> 
> Paolo, does b2g use the new downloads API?

Yes we do.
Assignee

Comment 34

5 years ago
Posted patch 20141127_safebrowsing.patch (obsolete) — Splinter Review
(In reply to Gian-Carlo Pascutto [:gcp] from comment #28)
> ::: b2g/app/b2g.js
> @@ +358,5 @@
> > +pref("browser.safebrowsing.reportPhishURL", "http://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%");
> > +pref("browser.safebrowsing.reportMalwareURL", "http://%LOCALE%.malware-report.mozilla.com/?hl=%LOCALE%");
> > +pref("browser.safebrowsing.reportMalwareErrorURL", "http://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%");
> > +pref("browser.safebrowsing.appRepURL", "https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%");
> > +pref("browser.safebrowsing.b2g.clientId", "Firefox");
> 
> Why is this needed? Why not simply set the existing pref (see line below
> this).
> 
> @@ +367,5 @@
> > +pref("browser.safebrowsing.id", "navclient-auto-ffox");
> > +#endif
> > +
> > +// Tables for application reputation.
> > +pref("urlclassifier.downloadBlockTable", "goog-badbinurl-shavar");
> 
> Does the B2G browser actually have a download system that can use these
> 
> ::: toolkit/components/url-classifier/SafeBrowsing.jsm
> @@ +130,5 @@
> > +    if (clientID == "B2G") {
> > +      try {
> > +        clientID = Services.prefs.getCharPref("browser.safebrowsing.b2g.clientId");
> > +      } catch(e) {}
> > +    }
> 
> Again, not sure why B2G needs a separate pref if a pref already exists for
> this.

It had be changed to use existing pref.
 
> ::: toolkit/components/url-classifier/content/listmanager.js
> @@ +16,4 @@
> >  //      that the listmanagers tables are properly written on updates
> >  
> >  // Log only if browser.safebrowsing.debug is true
> > +this.log = function log(...stuff) {
> 
> Why are these changes needed?
> 
> ::: toolkit/components/url-classifier/content/moz/alarm.js
> @@ +130,5 @@
> >    G_Alarm.call(this, callback, delayMS, opt_repeating, opt_maxTimes);
> >    this.debugZone = "conditionalalarm";
> >  }
> >  
> > +G_ConditionalAlarm.inherits = function(parentCtor) {
> 
> Same question. Why does .inherits suddenly have to be defined? What makes
> B2G so special here?
> 
> I'm not an expert in JavaScript but requiring changes to these files which
> are literally years old for no direct reason that I can see makes me very,
> very nervous.

This would be needed to use a shared global.
( see Fabrice's comment #30 )
Attachment #8502201 - Attachment is obsolete: true
Attachment #8516508 - Attachment is obsolete: true
Attachment #8525123 - Attachment is obsolete: true
Attachment #8529493 - Flags: review?(gpascutto)
Comment on attachment 8529493 [details] [diff] [review]
20141127_safebrowsing.patch

Review of attachment 8529493 [details] [diff] [review]:
-----------------------------------------------------------------

Did you test the feature? Do the warning pages work correctly?

On Android the updates only end up running while the browser is in memory, which doesn't tend to last very long. This means we don't update (and risk using metered/paid data) if we're not being used.

On Firefox OS this (I guess?) would always be active and I could see this ending up using some of the users metered data, which may be unexpected. I think it would be good to file a follow up bug and check the behaviors once this patch lands.

::: b2g/app/b2g.js
@@ +361,5 @@
> +pref("browser.safebrowsing.appRepURL", "https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%");
> +
> +#ifdef MOZILLA_OFFICIAL
> +// Normally the "client ID" sent in updates is appinfo.name, but for
> +// official Firefox releases from Mozilla we use a special identifier.

Remove the #ifdef and the comment. The "special identifier" is "navclient-auto-ffox" but that's a desktop only thing. (Where no-one really remembers the reason for it)

Just set the pref to "Firefox" unconditionally.
Attachment #8529493 - Flags: review?(gpascutto) → review+
Assignee

Comment 36

5 years ago
Thanks comment, gcp

(In reply to Gian-Carlo Pascutto [:gcp] from comment #35)
> Did you test the feature? Do the warning pages work correctly?
Actually, the warning pages are not working correctly, because, in master branch, other warning pages( i.e. certificate error, offline error, ... ) ALSO couldn't be worked correctly.

see bug 1056620, bug 1083640

net_error.js( gaia/apps/system/js/net_error.js) have got some errors( i.e. offline, DNS not found, file not found, and also, malware, phissing ).
In August, we could get some errors from gecko in net_error.js.
Although, in current master branch, we couldn't get it.

Then, my patch will be publish safebrowsing event( malware, phissing ) to gaia part, but no one will be get that event on gaia part, so, the warning pages will not work correctly...

> On Android the updates only end up running while the browser is in memory,
> which doesn't tend to last very long. This means we don't update (and risk
> using metered/paid data) if we're not being used.
> 
> On Firefox OS this (I guess?) would always be active and I could see this
> ending up using some of the users metered data, which may be unexpected. I
> think it would be good to file a follow up bug and check the behaviors once
> this patch lands.
> 
> ::: b2g/app/b2g.js
> @@ +361,5 @@
> > +pref("browser.safebrowsing.appRepURL", "https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%");
> > +
> > +#ifdef MOZILLA_OFFICIAL
> > +// Normally the "client ID" sent in updates is appinfo.name, but for
> > +// official Firefox releases from Mozilla we use a special identifier.
> 
> Remove the #ifdef and the comment. The "special identifier" is
> "navclient-auto-ffox" but that's a desktop only thing. (Where no-one really
> remembers the reason for it)
> 
> Just set the pref to "Firefox" unconditionally.
got it, may I get your review to my patch?
Attachment #8529632 - Flags: review?(gpascutto)
Attachment #8529632 - Flags: review?(gpascutto) → review+
Attachment #8529493 - Attachment is obsolete: true
Assignee

Comment 37

5 years ago
Thanks
Flags: needinfo?(paolo.mozmail)
Keywords: checkin-needed
Assignee

Comment 38

5 years ago
Thanks review gcp,

Do we need the feature to switch enable/disable safebrowsing on B2G?
If it might be needed, it might be have to file a new bug.
Flags: needinfo?(gpascutto)
Hi Yusuke, can you provide a try run for this changes, thanks!
Flags: needinfo?(yu-yamamoto)
Assignee

Comment 40

5 years ago
Hi Carsten,
It's working good. The malware site have been blocked in FxOS's Browser if we are visiting malware sites.
# it was confirmed with mozilla's "it's a trap page".
# http://www.itisatrap.org/firefox/its-a-trap.html
Flags: needinfo?(cbook)
Assignee

Updated

5 years ago
Flags: needinfo?(yu-yamamoto)
Reporter

Comment 41

5 years ago
Yusuke, can you rebase your patch? I'll like to push it to try before landing? thanks!
Assignee

Comment 42

5 years ago
Posted patch 20141208_safebrowsing.patch (obsolete) — Splinter Review
Fabrice, this is the rebased patch file, may I have your review with my attached file?
Reporter

Comment 43

5 years ago
(In reply to Yusuke Yamamoto from comment #42)
> Created attachment 8533019 [details] [diff] [review]
> 20141208_safebrowsing.patch
> 
> Fabrice, this is the rebased patch file, may I have your review with my
> attached file?

Hum, against which branch did you rebase? That still doesn't apply on a hg repo.
Assignee

Comment 44

5 years ago
(In reply to Fabrice Desré [:fabrice] from comment #43)
> (In reply to Yusuke Yamamoto from comment #42)
> > Created attachment 8533019 [details] [diff] [review]
> > 20141208_safebrowsing.patch
> > 
> > Fabrice, this is the rebased patch file, may I have your review with my
> > attached file?
> 
> Hum, against which branch did you rebase? That still doesn't apply on a hg
> repo.
oh, I just didn't it on hg repository...
I'll try it.

Please wait for that for some hours.
# There are no hg sync repository on my PC, so it will be have some times to sync it...
(In reply to Yusuke Yamamoto from comment #40)
> Hi Carsten,
> It's working good. The malware site have been blocked in FxOS's Browser if
> we are visiting malware sites.
> # it was confirmed with mozilla's "it's a trap page".
> # http://www.itisatrap.org/firefox/its-a-trap.html

This tells you whether the blocking is working, but it doesn't tell you anything whether updates actually work, because that page (itisatrap.org) is inserted locally in SafeBrowsing.jsm. Ideally you want to let it run for a while, then find a real phishing page that is warned on desktop (e.g. via PhishTank) and see if b2g warns as well.

>Do we need the feature to switch enable/disable safebrowsing on B2G?
>If it might be needed, it might be have to file a new bug.

See comment 35. I think not all users would be happy to have the updates use up their metered data plans.
Flags: needinfo?(gpascutto)
Assignee

Comment 46

5 years ago
(In reply to Gian-Carlo Pascutto [:gcp] from comment #45)
> (In reply to Yusuke Yamamoto from comment #40)
> > Hi Carsten,
> > It's working good. The malware site have been blocked in FxOS's Browser if
> > we are visiting malware sites.
> > # it was confirmed with mozilla's "it's a trap page".
> > # http://www.itisatrap.org/firefox/its-a-trap.html
> 
> This tells you whether the blocking is working, but it doesn't tell you
> anything whether updates actually work, because that page (itisatrap.org) is
> inserted locally in SafeBrowsing.jsm. Ideally you want to let it run for a
> while, then find a real phishing page that is warned on desktop (e.g. via
> PhishTank) and see if b2g warns as well.
Oh, that makes sense.
I'll have checked the malware page you suggested me.
Then, after that, I'll rebase the patch.

> >Do we need the feature to switch enable/disable safebrowsing on B2G?
> >If it might be needed, it might be have to file a new bug.
> 
> See comment 35. I think not all users would be happy to have the updates use
> up their metered data plans.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #35)
> I think it would be good to file a follow up bug and check the behaviors once
> this patch lands.
I got it.
Flags: needinfo?(cbook)
Assignee

Comment 47

5 years ago
Posted patch 20141209_safebrowsing.patch (obsolete) — Splinter Review
Hi gcp, Fabrice,

I have checked safebrowsing feature with new malware web site( was found on PhishTank web site ), can see the warning page.

Then, I rebased the safebrowsing path.
Attachment #8533019 - Attachment is obsolete: true
Reporter

Comment 49

5 years ago
A few points to clear before landing:
- B2G ICS Emulator builds are failing.
- I'd like to know the memory impact on device when turning this on (use get_about_memory.py)
- In my tests, going to a phishing site leads to displaying "URL not valid, The URL entered is not valid and cannot be loaded" which is not good enough. We need to display something similar to the "Reported Web Forgery!" desktop page.
Assignee

Comment 50

5 years ago
Sorry to reply too late.
But, I'd like to you to wait the patch about a week.
I'm in busy my real work..

(In reply to Fabrice Desré [:fabrice] from comment #49)
> A few points to clear before landing:
> - B2G ICS Emulator builds are failing.
I got the failing cause, then, I've been coding.

> - I'd like to know the memory impact on device when turning this on (use
> get_about_memory.py)
OK.
I'll try it.

> - In my tests, going to a phishing site leads to displaying "URL not valid,
> The URL entered is not valid and cannot be loaded" which is not good enough.
> We need to display something similar to the "Reported Web Forgery!" desktop
> page.
I've been trying to find the reason, but, I just got a part of cause... 
URLClassifier ( gecko side ) send the phissing error code to other gecko part ( then, finally this error code will reach to gaia part).
Although, in gaia part, net_error.js( this module displays some error pages. e.g. phissing, offline... ) is got the url not-valid error instead of phissing error.

Probably, the someone on gecko who is in between URLClassifier and net_error.js, is rounding the error code from phissing to url not-valid.
Then, the display is got the "URL not valid".

But, it's hard for me to find "someone on gecko part".
Assignee

Comment 52

5 years ago
Posted patch BuildError_for_ICS_Emulator (obsolete) — Splinter Review
(In reply to Fabrice Desré [:fabrice] from comment #49)
> A few points to clear before landing:
> - B2G ICS Emulator builds are failing.
this is a patch for build failing on ICS emulator.

> - I'd like to know the memory impact on device when turning this on (use
> get_about_memory.py)
I got the memory information file on Nexus-S.
Assignee

Comment 53

5 years ago
Posted file memory-reports (obsolete) —
Assignee

Comment 54

5 years ago
I'll update about this record, and then I've summarized currently situation as follows.

In Comment#49 ,
> A few points to clear before landing:
> - B2G ICS Emulator builds are failing.
attached file on this record is supported this failing error.

> - I'd like to know the memory impact on device when turning this on (use get_about_memory.py)
I attached memory reports on Comment#53.

> - In my tests, going to a phishing site leads to displaying "URL not valid, The URL entered is not valid and cannot be loaded" which is not good enough. We need to display something similar to the "Reported Web Forgery!" desktop page.
attached file on this record is supported the " URL not valid error ".

It's glad for me to get code-review with my patch.
Attachment #8533540 - Attachment is obsolete: true
Attachment #8542528 - Attachment is obsolete: true
Attachment #8547957 - Flags: review?(gpascutto)
Comment on attachment 8547957 [details] [diff] [review]
20150109_safebrowsing.patch

Review of attachment 8547957 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/Classifier.cpp
@@ +410,4 @@
>  
>    for (uint32_t i = 0; i < foundTables.Length(); i++) {
> +    table = foundTables[i];
> +    HashStore store(table, mStoreDirectory);

What's the reason for this change?
Assignee

Comment 56

5 years ago
The reason for that change is to support for ICS build error.
In only ICS, to use the variables directly, B2G Emulator builds are failing( see Comment 49 ).
Then, the patch is changed not to use the variables directly.
Comment on attachment 8547957 [details] [diff] [review]
20150109_safebrowsing.patch

Review of attachment 8547957 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/Classifier.cpp
@@ +410,4 @@
>  
>    for (uint32_t i = 0; i < foundTables.Length(); i++) {
> +    table = foundTables[i];
> +    HashStore store(table, mStoreDirectory);

Does moving the variable inside the loop, i.e. nsCString table(foundTables[i]); still compile? If so I prefer that.
Attachment #8547957 - Flags: review?(gpascutto) → review+
Assignee

Comment 58

5 years ago
(In reply to Gian-Carlo Pascutto [:gcp] from comment #57)
> Does moving the variable inside the loop, i.e. nsCString
> table(foundTables[i]); still compile? If so I prefer that.
thanks review, gcp.
I attached the re-coded patch. 

If your review would be OK with this patch, what should I do after that?
Attachment #8529632 - Attachment is obsolete: true
Attachment #8551544 - Flags: review?(gpascutto)
Attachment #8551544 - Flags: review?(gpascutto) → review+
>If your review would be OK with this patch, what should I do after that?

See comment 45. I guess you can land this patch but switch the defaults off in the mean time? 
i.e. in b2g.js
https://dxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js?from=b2g.js#346
https://dxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js?from=b2g.js#349
set both to false.
No need to request re-review for just that change.

It needs to be checked (and follow-up bugs filed if indeed):
a) Whether visiting http://www.itisatrap.org/firefox/its-a-trap.html triggers the correct warning page.
b) Whether visiting a real phishing site (e.g. from PhishTank) that desktop warns on, B2G also warns on (i.e. that that updates *from Google* work correctly)
c) What you want to do with the mobile (paid) data usage problem.

Comment 60

4 years ago
Attachment #8542529 - Attachment is obsolete: true

Comment 62

4 years ago
I acquired two memory reports with an attached patch.(A model is Nexus-s.)

a)Safe browsing ON
b)Safe browsing OFF

Would you confirm it?
Flags: needinfo?(fabrice)
Reporter

Comment 63

4 years ago
That looks fine... Can you rebase your patch with the prefs set as asked in comment 59 so we can land? thanks!
Flags: needinfo?(fabrice)
Assignee

Comment 64

4 years ago
Posted patch 20150212_safebrowsing.patch (obsolete) — Splinter Review
(In reply to Fabrice Desré [:fabrice] from comment #63)
> That looks fine... Can you rebase your patch with the prefs set as asked in
> comment 59 so we can land? thanks!
Hi Fabrice,
Thanks to your checking on our comment.
We've did it with this attached patch.
Assignee

Updated

4 years ago
Keywords: checkin-needed
Reporter

Updated

4 years ago
Keywords: checkin-needed
Reporter

Comment 65

4 years ago
(In reply to Yusuke Yamamoto from comment #64)
> Created attachment 8563310 [details] [diff] [review]
> 20150212_safebrowsing.patch
> 
> (In reply to Fabrice Desré [:fabrice] from comment #63)
> > That looks fine... Can you rebase your patch with the prefs set as asked in
> > comment 59 so we can land? thanks!
> Hi Fabrice,
> Thanks to your checking on our comment.
> We've did it with this attached patch.

Sorry, that still doesn't apply to a current trunk tree.
Assignee

Comment 66

4 years ago
(In reply to Fabrice Desré [:fabrice] from comment #65)
> (In reply to Yusuke Yamamoto from comment #64)
> > Created attachment 8563310 [details] [diff] [review]
> > 20150212_safebrowsing.patch
> > 
> > (In reply to Fabrice Desré [:fabrice] from comment #63)
> > > That looks fine... Can you rebase your patch with the prefs set as asked in
> > > comment 59 so we can land? thanks!
> > Hi Fabrice,
> > Thanks to your checking on our comment.
> > We've did it with this attached patch.
> 
> Sorry, that still doesn't apply to a current trunk tree.
Oh...
Actually, it's already night in Japan, so, I will do that tomorrow.
Assignee

Comment 67

4 years ago
Hi Fabrice,
This is the rebased and re-generated patch.
Could you confirm this?
Attachment #8563310 - Attachment is obsolete: true
Reporter

Comment 69

4 years ago
Thanks a lot Yusuke!
https://hg.mozilla.org/mozilla-central/rev/76c0924aea88
Assignee: nobody → yu-yamamoto
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S6 (20feb)
Assignee

Comment 71

4 years ago
Hi Fabrice,

I'm sure that this bug is already closed, but I'd like to talk about switching feature of safebrowsing on gaia side.

I mean, gecko part could switch to enable/disable safebrowsing feature on "browser.safebrowsing.enabled" prefs, but the users could not switch this feature because Gaia part(settings application) didn't have a "switch toggle".

I think that it would be helpful for users to have a switching toggle on settings application with safebrowsing.

Then, it may be good to discuss about the "toggle" on settings application.
Although, I didn't know what should I do...
If possible, may I have any suggestion?

P.S. if it's no-good to talk to continue on fixed bugzilla, could you point out.
Flags: needinfo?(fabrice)
Reporter

Comment 72

4 years ago
Let's open a new bug in the Gaia::Settings components. I'm not sure in which section we should put this switch, but I don't want it to be buried in the "developer" section. We'll have to ask UX what they think.
Flags: needinfo?(fabrice)
Flags: needinfo?(mrbkap)
Whiteboard: [adv-main38-]
You need to log in before you can comment on or make changes to this bug.