Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#4089 closed defect (fixed)

Move punycode.js to correct position for lib/adblockplus.js

Reported by: kzar Assignee: kzar
Priority: P1 Milestone: Adblock-Plus-1.12-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: sebastian, trev, Ross, scheer Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29345387/

Description (last modified by kzar)

Environment

Adblock Plus for Google Chrome 1.9.4.1498

How to reproduce

  1. Open the options page.
  2. Enter location.href = "qunit/index.html"; in the console.

Observed behaviour

Some tests fail with the message "Cannot read property 'toUnicode' of undefined".

Expected behaviour

Those tests should not fail.

Notes

  • It turns out the ordering for the lib/adblockplus.js [convert_js] section in adblockpluschrome/metadata.common is wrong. lib/punycode.js must be above lib/url.js.
  • This bug is also visible to the user when not using content blocking if the URL of a request or document uses an IDN domain, in which case the same exception is thrown and adverts are not blocked.

Hints for testers

  1. Open the console for the Adblock Plus background page. (Open extensions menu, enable developer mode and click "background page".)
  2. Browse to http://bücher.com (will will become http://xn--bcher-kva.com)
  3. Ensure that there are not TypeError: Cannot read property 'toUnicode' of undefined exceptions being thrown in the background page console.

Change History (13)

comment:1 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:2 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Issue 4089 - Fix punycode module ordering

comment:3 Changed 3 years ago by kzar

  • Cc Ross scheer added
  • Milestone set to Adblock-Plus-1.12-for-Chrome-Opera-Safari
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:4 Changed 3 years ago by sebastian

  • Description modified (diff)

comment:5 Changed 3 years ago by kzar

  • Description modified (diff)

comment:6 Changed 3 years ago by Ross

Running the above in the console results in lots of (pointing at various files):

chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/qunit/common.js net::ERR_FILE_NOT_FOUND

Should I be testing this in the Chrome extension or something else? What is Adblock Update?

ABP 1.11.0.1610
Chrome 48 / Ubuntu 14.04

comment:7 Changed 3 years ago by kzar

  • Description modified (diff)

I'm not sure what's happening there, the tests all pass for me. I'm going to try installing that development build now.

What is Adblock Update?

A typo, fixed now.

Should I be testing this in the Chrome extension or something else?

Yes in the Chrome extension.

comment:8 Changed 3 years ago by kzar

Hang on, which files are failing to load? I see some in the console that are just caused by the fact that the tests live inside the qunit directory. (subscriptions.xml, abp-19.png and abp-38.png.) If that's all you're seeing too then I don't think it's cause for concern.

comment:9 Changed 3 years ago by Ross

It's this list (ignoring the icons):

index.html:12 GET chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/qunit/qunit.css net::ERR_FILE_NOT_FOUND
index.html:13 GET chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/qunit/qunit.js net::ERR_FILE_NOT_FOUND
index.html:23 GET chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/qunit/common.js net::ERR_FILE_NOT_FOUND
index.html:24 GET chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/qunit/tests/adblockplus.js net::ERR_FILE_NOT_FOUND
index.html:25 GET chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/qunit/tests/versionComparator.js net::ERR_FILE_NOT_FOUND
index.html:26 GET chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/qunit/tests/url.js net::ERR_FILE_NOT_FOUND
index.html:27 GET chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/qunit/tests/signatures.js net::ERR_FILE_NOT_FOUND
index.html:28 GET chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/qunit/tests/cssEscaping.js net::ERR_FILE_NOT_FOUND

comment:10 Changed 3 years ago by kzar

OK I can also reproduce this when installing development builds from the website, I tried 1.11.0.1610, 1.11.0.1609, 1.11.0.1608 and 1.11.0.1607. Almost all the qunit files fail to load as Ross mentioned and the test page is pretty much completely broken.

I can't reproduce this when installing from a local devenv copy though, from there the tests run and now all pass.

My only guess is that some of the test files are deliberately not included in the builds to save space. I'm not sure about that though, hopefully Wladimir / Sebastian can confirm.

comment:11 Changed 3 years ago by kzar

  • Description modified (diff)

(In the hope that the problem running the qunit tests is to be expected I've added some alternative testing instructions.)

comment:12 Changed 3 years ago by trev

Yes, unit tests aren't meant to be run from the extension builds, merely from the development environment. Nor should testing an issue be done via unit tests, it should rather be reproduced in the real world. And in fact, when going back to a build where the issue isn't fixed yet I see lots of instances of that error message in the background page after navigating to http://россия.рф/.

The fact that qunit/index.html is included in the builds is actually a bug.

Last edited 3 years ago by trev (previous) (diff)

comment:13 Changed 3 years ago by trev

I filed #4096 on that bug.

Note: See TracTickets for help on using tickets.