no "meta redirection blocked" message

[z117]

Hi,
after upgrade from 2.9.0.14 to 2.9.5.1 the message "NoScript blocked a <META> redirection inside a <NOSCRIPT> element ..." does not appear regardless of corresponding checkbox setting.

Steps to Reproduce:
1. Upgrade noscript.
2. Disable meta redirection inside noscript element, enable corresponding message.
3. Go to a site with such a redirection. (In my case, it's http://[any livejournal user].livejournal.com/feed.)

Actual results:
No redirection, no message.

Expected results:
No redirection, but a message at the top of the page.

[barbaz]

When this issue occurs, do you see anything related in the Browser Console? (Ctrl-Shift-J)
(if you don't know what's related, turn off CSS warnings and post everything else you see)

[z117]

As far as I could see, with CSS off it is literally nothing - except

Password fields present on an insecure (http://) page. This is a security risk that allows user login credentials to be stolen.[Learn More]

which obviously has nothing to do with the issue.

[z117]

Wait, I see this

[NoScript]: ReferenceError: ctx is not defined notifying meta refresh at http://[a username deleted].livejournal.com/feed/

In the terminal window where firefox was started.

[z117]

Any activity on the issue?

[Thrawn]

Does the same message appear in the Browser Console?

If so, does it include a link to the JavaScript file that encountered the error? Name and line number would be quite helpful.

[dnolan]

This appears only once in the source code, in function 'processMetaRefresh': /xpi/chrome/content/noscript/MainChild.js:352.

The exception is thrown from inside the 'notifyCallback' callback. The only caller of 'processMetaRefresh' is at /xpi/chrome/content/noscript/UISync.jsm:98, function 'onContentLoad'. There we see 'notifyCallback' is actually 'notifyMetaRefresh', just above.

I'm guessing the bug is in trying to use this 'non-static' member function without binding it to the appropriate UISync.

Here's an untested patch:

Code: Select all

diff --git a/xpi/chrome/content/noscript/UISync.jsm b/xpi/chrome/content/noscript/UISync.jsm
index 343d7c5..d83819e 100644
--- a/xpi/chrome/content/noscript/UISync.jsm
+++ b/xpi/chrome/content/noscript/UISync.jsm
@@ -95,7 +95,7 @@ UISync.prototype = {
           let url = doc.URL;
           let jsBlocked = /^https?:/.test(url) && !ns.isJSEnabled(ns.getSite(url), w);
           if (jsBlocked) {
-            ns.processMetaRefresh(doc, this.notifyMetaRefresh);
+            ns.processMetaRefresh(doc, this.notifyMetaRefresh.bind(this));
             w.addEventListener("pageshow", ev => this.onPageShowNS(ev), false);
           }
         } else {

[barbaz]

This appears only once in the source code, in function 'processMetaRefresh': /xpi/chrome/content/noscript/MainChild.js:352.

The exception is thrown from inside the 'notifyCallback' callback. The only caller of 'processMetaRefresh' is at /xpi/chrome/content/noscript/UISync.jsm:98, function 'onContentLoad'. There we see 'notifyCallback' is actually 'notifyMetaRefresh', just above.

Nice find, thanks for investigating.

I'm guessing the bug is in trying to use this 'non-static' member function without binding it to the appropriate UISync.

Here's an untested patch:

I haven't tested either, but it looks to me more like UISync.jsm is just missing this line in a couple places -

Code: Select all

let ctx = this.ctx;

Specifically, in the functions unwire and notifyMetaRefresh.

[dnolan]

I haven't tested either, but it looks to me more like UISync.jsm is just missing this line in a couple places -

Code: Select all

let ctx = this.ctx;

Specifically, in the functions unwire and notifyMetaRefresh.

Umm, I think you would know better than me, I don't really *know* javascript (I hate it too much) so I simply imagined that using 'ctx' was the same as 'this.ctx' (a la C++). Regardless, I think binding the method is also necessary, though maybe Giorgio prefers the lambda style (as in the next line).