Ublock: Logger incorrectly shows all popups as blocked by one filter.

[kasper93]

One or more specific URLs where the issue occurs

http://raymondhill.net/ublock/popup.html

Screenshot in which the issue can be seen

Steps for anyone to reproduce the issue

1. Open any page with popups.
2. Trigger popup that will get blocked.
3. Trigger popup again.

After first blocked popup every other popup that gets blocked or not will be shown as blocked by filter used to block first popup. See the screenshot where about:blank is not blocked and it is shown as blocked by the logger.

Your settings

• Browser/version: Firefox Nightly
• uBlock Origin version: 1.13.7rc1

[gorhill]

Pretty sure it's a regression from https://github.com/gorhill/uBlock/commit/02323826950c7d99b65d93f9edb8fa018159973c#diff-ce215d94a52d449f1a34f292126608d7.

[kasper93]

@gorhill: Actually it still fails. popunderMatch() calls popupMatch(), which can set logData, but mapPopunderResult() may reject the result and logData will still be defined.

I did this to clear logData if we have no match. If result is non zero we have valid logData else we just discard whatever we had.

diff --git a/src/js/tab.js b/src/js/tab.js
index 95003969..0c200211 100644
--- a/src/js/tab.js
+++ b/src/js/tab.js
@@ -639,6 +639,7 @@ vAPI.tabs.onPopupUpdated = (function() {

     var mapPopunderResult = function(popunderURL, popunderHostname, result) {
         if (
+            result === 0 ||
             logData === undefined ||
             logData.source !== 'static' ||
             logData.token === µb.staticNetFilteringEngine.noTokenHash
@@ -650,11 +651,11 @@ vAPI.tabs.onPopupUpdated = (function() {
         }
         var re = new RegExp(logData.regex),
             matches = re.exec(popunderURL);
-        if ( matches === null ) { return ''; }
+        if ( matches === null ) { return 0; }
         var beg = matches.index,
             end = beg + matches[0].length,
             pos = popunderURL.indexOf(popunderHostname);
-        if ( pos === -1 ) { return ''; }
+        if ( pos === -1 ) { return 0; }
         // https://github.com/gorhill/uBlock/issues/1471
         //   We test whether the opener hostname as at least one character
         //   within matched portion of URL.
@@ -704,9 +705,6 @@ vAPI.tabs.onPopupUpdated = (function() {
     };

     return function(targetTabId, openerTabId) {
-        // https://github.com/gorhill/uBlock/issues/2776
-        logData = undefined;
-
         // Opener details.
         var tabContext = µb.tabContextManager.lookup(openerTabId);
         if ( tabContext === null ) { return; }
@@ -753,6 +751,10 @@ vAPI.tabs.onPopupUpdated = (function() {

         // Log only for when there was a hit against an actual filter (allow or block).
         if ( µb.logger.isEnabled() ) {
+            if ( result === 0 ) {
+                // https://github.com/gorhill/uBlock/issues/2776
+                logData = undefined;
+            }
             µb.logger.writeOne(
                 popupType === 'popup' ? openerTabId : targetTabId,
                 'net',

[kasper93]

I think result check in mapPopunderResult() would be more robust than relying that logData is cleared every time. But nevermind.

I'm still worried about this "second try" popunder match which will not be executed even when it should. You check result !== 0 https://github.com/gorhill/uBlock/blob/7fb034f6403a6eec29c7f4946d28603510a86f71/src/js/tab.js#L690-L692 but mapPopunderResult() can return '' in some code path. This looks like unwanted change in logic from refactoring commit, and will return too early from the popunderMatch() function.

[gorhill]

I'm still worried about this "second try" popunder match which will not be executed even when it should.

Why should it? A match has been found, no point trying to find another one.

[kasper93]

Why should it? A match has been found, no point trying to find another one.

I'm talking about the case when the match is not found. The https://github.com/gorhill/uBlock/issues/1598 case.

These both cases will return '' which will be wrongly interpreted:

https://github.com/gorhill/uBlock/blob/7fb034f6403a6eec29c7f4946d28603510a86f71/src/js/tab.js#L653

if ( matches === null ) { return ''; }

https://github.com/gorhill/uBlock/blob/7fb034f6403a6eec29c7f4946d28603510a86f71/src/js/tab.js#L657

if ( pos === -1 ) { return ''; }

---

popunderMatch() will return because '' !== 0, this will be incorrectly detected as a match here
https://github.com/gorhill/uBlock/blob/7fb034f6403a6eec29c7f4946d28603510a86f71/src/js/tab.js#L690-L692

if ( result !== 0 )

---

Before the changes in
https://github.com/gorhill/uBlock/commit/02323826950c7d99b65d93f9edb8fa018159973c#diff-ce215d94a52d449f1a34f292126608d7
the check was obviously result !== '' now that you return integer, two return patch are incorrectly handled.

[gorhill]

if ( matches === null ) { return ''; }

This is definitely wrong, I don't know how I missed this. This should be return 0; following refactoring. Thanks for catching this.

[kasper93]

Damn, now I remember why I've added

diff --git a/src/js/tab.js b/src/js/tab.js
index 1454cc16..1a385236 100644
--- a/src/js/tab.js
+++ b/src/js/tab.js
@@ -639,6 +639,7 @@ vAPI.tabs.onPopupUpdated = (function() {

     var mapPopunderResult = function(popunderURL, popunderHostname, result) {
         if (
+            result === 0 ||
             logData === undefined ||
             logData.source !== 'static' ||
             logData.token === µb.staticNetFilteringEngine.noTokenHash

When first call to mapPopunderResult() returns 0, but popupMatch() have returned !== 0 meaning logData is set, second call to mapPopunderResult() might use previous (from the first call) logData, if second popupMatch() returns 0 i.e not set logData.

Still the end result will be 0, because result is 0, but mapPopunderResult() will do some unnecessary work instead of returning at beginning.