Issue 29863604: Issue 6865 - Update ABP dependency to version 3.2

Issue 29863604: Issue 6865 - Update ABP dependency to version 3.2 (Closed)

Created:
Aug. 24, 2018, 8:38 p.m. by diegocarloslima

Modified:
Jan. 21, 2019, noon

Reviewers:
Thomas Greiner, jens

CC:
anton

Visibility:
Public.

Description

Issue 6865 - Update ABP dependency to version 3.2

Patch Set 2 : Removing patch file #

Total comments: 22

Patch Set 3 : Adjustments based on comments #

Patch Set 4 : Removing allowed contexts #

Patch Set 5 : Adjusting build script and removing extension first run page #

Total comments: 8

Patch Set 6 : Adjusting code style #

Created: Jan. 16, 2019, 1:45 p.m.

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+433 lines, -471 lines)			Patch
M	adblockplus/build.py	View	1 2 3 4	1 chunk	+40 lines, -26 lines	0 comments	Download
M	adblockplus/extensionBridge.js	View	1 2 3 4 5	1 chunk	+177 lines, -310 lines	0 comments	Download
M	adblockplus/issue-6070.patch	View		2 chunks	+6 lines, -6 lines	0 comments	Download
R	adblockplus/issue-6108.patch	View	1	1 chunk	+0 lines, -42 lines	0 comments	Download
A	adblockplus/issue-6865.patch	View	1 2 3 4	1 chunk	+24 lines, -0 lines	0 comments	Download
M	adblockplus/metadata.gecko	View		1 chunk	+3 lines, -3 lines	0 comments	Download
M	dependencies	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	ensure_dependencies.py	View		7 chunks	+75 lines, -8 lines	0 comments	Download
M	mobile/android/base/java/org/adblockplus/browser/AbpCheckBoxPreference.java	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	mobile/android/base/java/org/adblockplus/browser/BrowserAppUtils.java	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	mobile/android/base/java/org/adblockplus/browser/ExtensionBridge.java	View		5 chunks	+18 lines, -18 lines	0 comments	Download
M	mobile/android/base/java/org/adblockplus/browser/MoreSubscriptionsPreferenceGroup.java	View	1 2 3 4 5	5 chunks	+9 lines, -9 lines	0 comments	Download
M	mobile/android/base/java/org/adblockplus/browser/SubscriptionContainer.java	View	1 2 3 4 5	5 chunks	+21 lines, -28 lines	0 comments	Download
M	mobile/android/base/java/org/adblockplus/browser/SubscriptionPreferenceCategory.java	View	1 2 3 4 5	3 chunks	+4 lines, -5 lines	0 comments	Download
M	mobile/android/base/java/org/adblockplus/browser/WhitelistedWebsitesPreferenceGroup.java	View		3 chunks	+3 lines, -3 lines	0 comments	Download
M	mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	mobile/android/base/moz.build	View		1 chunk	+1 line, -1 line	0 comments	Download
M	mobile/android/chrome/content/browser.js	View		1 chunk	+0 lines, -4 lines	0 comments	Download
M	toolkit/components/extensions/ext-c-runtime.js	View	1 2	1 chunk	+22 lines, -0 lines	0 comments	Download
M	toolkit/components/extensions/schemas/runtime.json	View	1 2 3	1 chunk	+22 lines, -0 lines	0 comments	Download

Messages

Total messages: 9

Expand All Messages | Collapse All Messages

anton

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensionBridge.js File adblockplus/extensionBridge.js (right): https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensionBridge.js#newcode53 adblockplus/extensionBridge.js:53: do we need this empty line? https://codereview.adblockplus.org/29863604/diff/29863624/mobile/android/base/java/org/adblockplus/browser/SubscriptionContainer.java File mobile/android/base/java/org/adblockplus/browser/SubscriptionContainer.java ...

Aug. 27, 2018, 5:57 a.m. (2018-08-27 05:57:10 UTC) #2

Thomas Greiner

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/build.py File adblockplus/build.py (right): https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/build.py#newcode33 adblockplus/build.py:33: "subscriptions.xml") The canonical version of subscriptions.xml is located in ...

Aug. 27, 2018, 3:24 p.m. (2018-08-27 15:24:42 UTC) #3

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/build.py
File adblockplus/build.py (right):

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/build.p...
adblockplus/build.py:33: "subscriptions.xml")
The canonical version of subscriptions.xml is located in adblockpluscore. The
copy in adblockplusui is only meant for testing the UI and should therefore not
be used in production.

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
File adblockplus/extensionBridge.js (right):

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
adblockplus/extensionBridge.js:42:
browser.runtime.abbSendRequest("Abb:OnLoaded");
Detail: The "Abb:" prefix seems redundant here since we're already calling
`abbSendRequest()`. Instead, we could just prepend it in ext-c-runtime.js.

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
adblockplus/extensionBridge.js:73: return successData(getAllSubscriptions());
The previous version didn't return anything but I couldn't find where this
function is being used to check whether the return value is actually being used.

My assumption would be that it isn't since the `getAllSubscriptions()` function
is preceded by a "remove" comment. If that's the case let's remove the return
value and the `getAllSubscriptions()` function.

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
adblockplus/extensionBridge.js:171: sub.filters.forEach(function(filter) {
Suggestion: If you want you can use a for-of loop instead of
`Array.prototype.forEach()`.

i.e. for (let filter of filters)

That way those loops will also be compatible with `Set` when we change the
implementation of the data structure in Core (see #6559).

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
adblockplus/extensionBridge.js:180: function flatIt(array)
Detail: You can more efficiently implement that using

Array.prototype.concat.apply([], array)

at least until Firefox 62 which will add `Array.prototype.flat()` so we could
even polyfill it until then.

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
adblockplus/extensionBridge.js:247: if (errorMsg) data.errorMsg = errorMsg;
The endpoint in ext-c-runtime.js expects this property to be called "error", not
"errorMsg".

Also the success value doesn't appear to be accessed in ext-c-runtime.js so
maybe I'm just misunderstanding how the implementation works. So feel free to
ignore my comment in case it doesn't apply.

https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/...
File toolkit/components/extensions/schemas/runtime.json (right):

https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/...
toolkit/components/extensions/schemas/runtime.json:164: "allowedContexts":
["content", "devtools"],
From what I understood we only need to communicate with the background page so
instead of exposing it to those other contexts - and thereby increase the risk
of opening up a security hole - what about just checking in extensionBridge.js
whether the API is available before using it?

https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/...
toolkit/components/extensions/schemas/runtime.json:165: "description": "Function
required to listen to Android requests. See
https://issues.adblockplus.org/ticket/6865",
Suggestion: The naming confused me at first because "Android requests" makes it
sound like it's about network requests that are passed on by the OS. However,
it's about passing messages between Adblock Browser and the extension.

So what about saying that and use consistent naming with the rest of the web
extension API? Because note that `chrome.extension.sendRequest()` has been
deprecated in favor of `chrome.runtime.sendMessage()`.

i.e.
Renaming `abbRegisterRequestListener()` to `onAbbMessage.addListener()` (see
also `onMessage.addListener()` and `onMessageExternal.addListener()`)
and renaming `abbSendRequest()` to `sendAbbMessage()` (see also `sendMessage()`
and `sendNativeMessage()`).

diegocarloslima

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/build.py File adblockplus/build.py (right): https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/build.py#newcode33 adblockplus/build.py:33: "subscriptions.xml") On 2018/08/27 15:24:41, Thomas Greiner wrote: > The ...

Aug. 27, 2018, 9:19 p.m. (2018-08-27 21:19:17 UTC) #4

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/build.py
File adblockplus/build.py (right):

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/build.p...
adblockplus/build.py:33: "subscriptions.xml")
On 2018/08/27 15:24:41, Thomas Greiner wrote:
> The canonical version of subscriptions.xml is located in adblockpluscore. The
> copy in adblockplusui is only meant for testing the UI and should therefore
not
> be used in production.

Acknowledged.

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
File adblockplus/extensionBridge.js (right):

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
adblockplus/extensionBridge.js:42:
browser.runtime.abbSendRequest("Abb:OnLoaded");
On 2018/08/27 15:24:41, Thomas Greiner wrote:
> Detail: The "Abb:" prefix seems redundant here since we're already calling
> `abbSendRequest()`. Instead, we could just prepend it in ext-c-runtime.js.

Acknowledged.

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
adblockplus/extensionBridge.js:53: 
On 2018/08/27 05:57:10, anton wrote:
> do we need this empty line?

Acknowledged.

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
adblockplus/extensionBridge.js:73: return successData(getAllSubscriptions());
On 2018/08/27 15:24:41, Thomas Greiner wrote:
> The previous version didn't return anything but I couldn't find where this
> function is being used to check whether the return value is actually being
used.
> 
> My assumption would be that it isn't since the `getAllSubscriptions()`
function
> is preceded by a "remove" comment. If that's the case let's remove the return
> value and the `getAllSubscriptions()` function.

Yeah, this was some test code that I forgot to remove ^^

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
adblockplus/extensionBridge.js:171: sub.filters.forEach(function(filter) {
On 2018/08/27 15:24:41, Thomas Greiner wrote:
> Suggestion: If you want you can use a for-of loop instead of
> `Array.prototype.forEach()`.
> 
> i.e. for (let filter of filters)
> 
> That way those loops will also be compatible with `Set` when we change the
> implementation of the data structure in Core (see #6559).

Acknowledged.

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
adblockplus/extensionBridge.js:180: function flatIt(array)
On 2018/08/27 15:24:41, Thomas Greiner wrote:
> Detail: You can more efficiently implement that using
> 
> Array.prototype.concat.apply([], array)
> 
> at least until Firefox 62 which will add `Array.prototype.flat()` so we could
> even polyfill it until then.

This was also some test code ^^

https://codereview.adblockplus.org/29863604/diff/29863624/adblockplus/extensi...
adblockplus/extensionBridge.js:247: if (errorMsg) data.errorMsg = errorMsg;
On 2018/08/27 15:24:42, Thomas Greiner wrote:
> The endpoint in ext-c-runtime.js expects this property to be called "error",
not
> "errorMsg".
> 
> Also the success value doesn't appear to be accessed in ext-c-runtime.js so
> maybe I'm just misunderstanding how the implementation works. So feel free to
> ignore my comment in case it doesn't apply.

Acknowledged.

https://codereview.adblockplus.org/29863604/diff/29863624/mobile/android/base...
File mobile/android/base/java/org/adblockplus/browser/SubscriptionContainer.java
(right):

https://codereview.adblockplus.org/29863604/diff/29863624/mobile/android/base...
mobile/android/base/java/org/adblockplus/browser/SubscriptionContainer.java:44:
private final Semaphore entriesReady = new Semaphore(0);
On 2018/08/27 05:57:10, anton wrote:
> since we don't do `acquire` and `release` on it do we still need it?

The Semaphore is still being used in other places, but I plan to refactor this
class in a follow up issue to remove the need of using a Semaphore.

https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/...
File toolkit/components/extensions/schemas/runtime.json (right):

https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/...
toolkit/components/extensions/schemas/runtime.json:164: "allowedContexts":
["content", "devtools"],
On 2018/08/27 15:24:42, Thomas Greiner wrote:
> From what I understood we only need to communicate with the background page so
> instead of exposing it to those other contexts - and thereby increase the risk
> of opening up a security hole - what about just checking in extensionBridge.js
> whether the API is available before using it?

If I dont add at least the 'content' context, I cant actually call those
functions from extensionBridge.js . I understand that it could be a potential
security breach, but since we're in control of the extensions that are installed
on ABB (for instance, we only install ABP), this shouldnt be a big concern. For
the 'devtools' context, it might not be needed, just added because all the other
functions had it, so I didnt want to have some unexpected edge case behavior

https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/...
toolkit/components/extensions/schemas/runtime.json:165: "description": "Function
required to listen to Android requests. See
https://issues.adblockplus.org/ticket/6865",
On 2018/08/27 15:24:42, Thomas Greiner wrote:
> Suggestion: The naming confused me at first because "Android requests" makes
it
> sound like it's about network requests that are passed on by the OS. However,
> it's about passing messages between Adblock Browser and the extension.
> 
> So what about saying that and use consistent naming with the rest of the web
> extension API? Because note that `chrome.extension.sendRequest()` has been
> deprecated in favor of `chrome.runtime.sendMessage()`.
> 
> i.e.
> Renaming `abbRegisterRequestListener()` to `onAbbMessage.addListener()` (see
> also `onMessage.addListener()` and `onMessageExternal.addListener()`)
> and renaming `abbSendRequest()` to `sendAbbMessage()` (see also
`sendMessage()`
> and `sendNativeMessage()`).

I will rename 'abbSendRequest' to 'sendAbbMessage', but for the listener, I
prefer to keep it as a function, if I add it as another property that has a
function, it would require more changes in the upstream code than the actual
solution, which is not desired. I will keep it as a function, but will rename it
to 'registerAbbMessageListener'

Thomas Greiner

https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/extensions/schemas/runtime.json File toolkit/components/extensions/schemas/runtime.json (right): https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/extensions/schemas/runtime.json#newcode164 toolkit/components/extensions/schemas/runtime.json:164: "allowedContexts": ["content", "devtools"], On 2018/08/27 21:19:17, diegocarloslima wrote: > ...

Aug. 28, 2018, 10:36 a.m. (2018-08-28 10:36:07 UTC) #5

https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/...
File toolkit/components/extensions/schemas/runtime.json (right):

https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/...
toolkit/components/extensions/schemas/runtime.json:164: "allowedContexts":
["content", "devtools"],
On 2018/08/27 21:19:17, diegocarloslima wrote:
> If I dont add at least the 'content' context, I cant actually call those
> functions from extensionBridge.js .

That's odd because "content" seems to refer to content scripts (i.e. the
extension code that is running in the context of a web page) and not to the
background page which we want to communicate with.

Unfortunately, I couldn't find a proper documentation for it other than what's
mentioned at
https://wiki.mozilla.org/WebExtensions/Implementing_APIs_out-of-process#shoul....
But there appear to be other API functions which don't specify the
"allowedContexts" property (e.g.
https://github.com/mozilla/gecko-dev/blob/master/toolkit/components/extension...).

> I understand that it could be a potential
> security breach, but since we're in control of the extensions that are
installed
> on ABB (for instance, we only install ABP), this shouldnt be a big concern.
For
> the 'devtools' context, it might not be needed, just added because all the
other
> functions had it, so I didnt want to have some unexpected edge case behavior

Note that it's not about whether a different extension could use this API but
about us exposing an API to a part of our extension that is directly interacting
with untrusted content (i.e. the web page). So it'd be great if we could somehow
limit our APIs to only the background page to sandbox it further.

https://codereview.adblockplus.org/29863604/diff/29863624/toolkit/components/...
toolkit/components/extensions/schemas/runtime.json:165: "description": "Function
required to listen to Android requests. See
https://issues.adblockplus.org/ticket/6865",
On 2018/08/27 21:19:17, diegocarloslima wrote:
> I will rename 'abbSendRequest' to 'sendAbbMessage', but for the listener, I
> prefer to keep it as a function, if I add it as another property that has a
> function, it would require more changes in the upstream code than the actual
> solution, which is not desired. I will keep it as a function, but will rename
it
> to 'registerAbbMessageListener'

Sounds good. :)

jens

Just minor remarks for the Java part. For the JS part it would be good ...

Jan. 10, 2019, 2:11 p.m. (2019-01-10 14:11:00 UTC) #6

Thomas Greiner

LGTM concerning the JavaScript. Only added a coding style-related comment. https://codereview.adblockplus.org/29863604/diff/29976555/adblockplus/extensionBridge.js File adblockplus/extensionBridge.js (right): https://codereview.adblockplus.org/29863604/diff/29976555/adblockplus/extensionBridge.js#newcode217 ...

Jan. 14, 2019, 4:38 p.m. (2019-01-14 16:38:14 UTC) #7

diegocarloslima

https://codereview.adblockplus.org/29863604/diff/29976555/adblockplus/extensionBridge.js File adblockplus/extensionBridge.js (right): https://codereview.adblockplus.org/29863604/diff/29976555/adblockplus/extensionBridge.js#newcode217 adblockplus/extensionBridge.js:217: return properties.every(function(item) { On 2019/01/14 16:38:13, Thomas Greiner wrote: ...

Jan. 16, 2019, 1:44 p.m. (2019-01-16 13:44:23 UTC) #8

jens

Jan. 16, 2019, 4:54 p.m. (2019-01-16 16:54:50 UTC) #9

On 2019/01/16 13:44:23, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29863604/diff/29976555/adblockplus/extensi...
> File adblockplus/extensionBridge.js (right):
> 
>
https://codereview.adblockplus.org/29863604/diff/29976555/adblockplus/extensi...
> adblockplus/extensionBridge.js:217: return properties.every(function(item) {
> On 2019/01/14 16:38:13, Thomas Greiner wrote:
> > Coding style: "Opening braces always go on their own line."
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29863604/diff/29976555/mobile/android/base...
> File
>
mobile/android/base/java/org/adblockplus/browser/MoreSubscriptionsPreferenceGroup.java
> (right):
> 
>
https://codereview.adblockplus.org/29863604/diff/29976555/mobile/android/base...
>
mobile/android/base/java/org/adblockplus/browser/MoreSubscriptionsPreferenceGroup.java:84:
> 
> On 2019/01/10 14:10:59, jens wrote:
> > Context could be final.
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29863604/diff/29976555/mobile/android/base...
> File
mobile/android/base/java/org/adblockplus/browser/SubscriptionContainer.java
> (right):
> 
>
https://codereview.adblockplus.org/29863604/diff/29976555/mobile/android/base...
>
mobile/android/base/java/org/adblockplus/browser/SubscriptionContainer.java:50:
> private SubscriptionContainer(final Context context)
> On 2019/01/10 14:10:59, jens wrote:
> > To be consistent, Context should be final in all constructors and in the
> > loadSubscriptions() method.
> 
> Acknowledged.
> 
>
https://codereview.adblockplus.org/29863604/diff/29976555/mobile/android/base...
> File
>
mobile/android/base/java/org/adblockplus/browser/SubscriptionPreferenceCategory.java
> (right):
> 
>
https://codereview.adblockplus.org/29863604/diff/29976555/mobile/android/base...
>
mobile/android/base/java/org/adblockplus/browser/SubscriptionPreferenceCategory.java:56:
> private static synchronized void initSubscriptions(Context context)
> On 2019/01/10 14:10:59, jens wrote:
> > Context could be final.
> 
> Acknowledged.

LGTM

Expand All Messages | Collapse All Messages