Use JSON for default.rulesets instead of XML by cschanaj · Pull Request #12273 · EFForg/https-everywhere

cschanaj · 2017-08-31T02:29:12Z

ping @Hainish

~~P.S. please do not feel obligated to merge before the next release.~~

ghost · 2017-08-31T02:42:07Z

@cschanaj You may be interested in https://github.com/koops76/https-everywhere-rules.

ghost · 2017-08-31T02:42:56Z

@Hainish @cschanaj I will extend on this PR, since I have a few ideas, specifically for xml2json Node script.

ghost · 2017-08-31T03:15:26Z

@cschanaj cschanaj#10

ghost · 2017-08-31T03:18:39Z

@cschanaj Opening all files at the same time may cause Node to run out of file handles on Windows. Will fix in a PR.

cschanaj · 2017-08-31T14:53:26Z

@Hainish For unknown reason, I got the following error message on Firefox 55.0.3 from the Browser Console despite the fact that I have not modify the file loading code. Do you have any suggestion?

P.S. The extension seems to work on Chromium now..
P.S. The colored part in the file URL is the current username.

ghost · 2017-08-31T18:56:23Z

@cschanaj You missed one of the places where rulesets file is loaded. See https://github.com/cschanaj/https-everywhere/pull/10/files#diff-81d05cf2685349d7bbbe9f394bd74506L652.

cschanaj · 2017-08-31T23:38:28Z

@koops76 fixed travis now. thanks! @Hainish the error still presist when installing the extension via the compile XPI file from makexpi.sh, but the extension is working now, why?

P.S. Feel free to review and merge this after testing. thanks!

ghost · 2017-09-01T02:29:47Z

@cschanaj Can you review my changes? Just wait until I will recreate the PR.

cschanaj · 2017-09-01T02:40:25Z

@koops76 specifically, what changes?

ghost · 2017-09-01T03:00:34Z

@cschanaj https://github.com/cschanaj/https-everywhere/pull/11
Not sure why some of your changes are also in here.

ghost · 2017-09-01T03:00:56Z

Feel free to not merge but copy the changes partially.

cschanaj · 2017-09-01T03:03:09Z

Not sure why some of your changes are also in here.

this is because you created your branch from cschanaj/master instead of cschanaj/fix-ruleset-loading

ghost · 2017-09-01T03:04:31Z

@cschanaj Nope: git checkout -b fix-ruleset-loading cschanaj/fix-ruleset-loading

ghost · 2017-09-01T03:07:18Z

@cschanaj Fixed: https://github.com/cschanaj/https-everywhere/pull/12

cschanaj · 2017-09-01T03:30:05Z

@koops76 please update the rewriter/rewrite.js in a separate PR. this is important but I don't want to include it here. thanks !!

ghost · 2017-09-01T03:33:22Z

@cschanaj ok

ghost · 2017-09-01T03:38:15Z

@cschanaj Done.

cschanaj · 2017-09-01T03:42:44Z

ping @Hainish this is ready for a review and merging. Simple benchmark shown that this reduce startup time (<1.5 seconds, 1.35 seconds on my machine) and memory usage (<100mb) effectively. thanks!

ghost · 2017-09-01T03:44:49Z

+
+# We make default.rulesets at build time, 
+# but it shouldn't have a variable timestamp
+subprocess.call(["touch", "-r", "src/install.rdf", ofn])


This is no longer needed, install.rdf is going away with WebExtensions transition.

This line is intended to remove the timestamp of default.rulesets. If src/install.rdf is gone, we will need another for timestamp reference. Let's see if Hainish would like to keep this line and suggest another file for timestamp reference.

P.S. Python because this will introduce fewer changes, I assume this will speed up the review process. thanks.

@cschanaj Still all Python scripts will be replaced by Node scripts soon.

ghost · 2017-09-01T03:45:33Z

Also why Python?

Hainish · 2017-09-01T18:08:39Z

@cowlicks let's review this together.

Hainish

Other than my comments, lgtm.

This is great, a huge performance and memory benefit. Great job!

Hainish · 2017-09-01T20:53:53Z

+    }
+  },
+
+  parseOneJsonRuleset: function(ruletag) {


Correspondingly, let's change the other one to parseOneXmlRuleset

Hainish · 2017-09-01T20:58:54Z

+        log(WARN, 'Error processing ruleset:' + e);	
+      }
+    }
+  },


Why use a counter to iterate rather than

for (let ruleset of ruleJson) { try { this.parseOneJsonRuleset(ruleset); } catch(e) { log(WARN, 'Error processing ruleset:' + e); } }

Hainish · 2017-09-01T21:15:17Z

+
+    var rules = ruletag["rule"];
+    for (let i in rules) {
+      let rule = rules[i];


Why not rule of rules?

Maybe not iterable, even though it should be.

Hainish · 2017-09-01T21:16:17Z

+    }
+
+    var exclusions = ruletag["exclusion"];
+    for (let i in exclusions) {


Why not exclusion of exclusions?

Maybe not iterable, even though it should be.

Hainish · 2017-09-01T21:16:48Z

+
+    var cookierules = ruletag["securecookie"];
+    for (let i in cookierules) {
+      let cookierule = cookierules[i];


Why not cookierule of cookierules?

Maybe not iterable, even though it should be.

Hainish · 2017-09-01T21:17:01Z

+
+    var targets = ruletag["target"];
+    for (let i in targets) {
+      let target = targets[i];


Why not target of targets?

Maybe not iterable, even though it should be.

Hainish · 2017-09-01T21:18:02Z

+subprocess.call(["touch", "-r", "src/install.rdf", ofn])

+# Everything is okay.
+print(" * Everything is Okay.")


cowlicks · 2017-09-01T22:20:53Z

I parsed master and this branch with:

function getJson() {
  out = [];
  all_rules.targets.forEach((k, target) => {
    out.push(JSON.stringify(target));
  });
  return out.toString();
}

And they look the same to me. Output is here.

ghost · 2017-09-02T00:01:56Z

I think non-iterable arrays should either be iterated using forEach or for (let i; i < array.length; i++) loop, not for (const i in array) loop.

cschanaj · 2017-09-02T00:13:51Z

@Hainish @koops76 checking through the code, I find that the console throw non-iterable error because ruletag.exclusion and ruletag.securecookie can be undefined when a ruleset does not specify the tags. Fixed by adding a conditional if. thanks!!

ghost · 2017-09-02T00:16:26Z

@cschanaj Always check if the array is undefined before attempting iteration.

cschanaj · 2017-09-02T00:17:17Z

Was fixed in d437c69

cowlicks · 2017-09-02T00:32:00Z

Huge memory improvements on startup here, with this PR, from chrome://system > mem_usage I get:

Extension [HTTPS Everywhere] 112 MB private, 56 MB shared, -1 FDs open of 1024

On master I get:

Extension [HTTPS Everywhere] 217 MB private, 57 MB shared, -1 FDs open of 1024

Where is this coming from? I'm guessing the DOM nodes from the original xml files are not cleaned up for a while.

Also worth noting, in my normal long-running browser session, I'm running HTTPS Everywhere : version 2017_8_31 and its memory usage is Extension [HTTPS Everywhere] 126 MB private, 63 MB shared, -1 FDs open of 1024. So the memory usage on master does decrease over time.

ghost · 2017-09-02T00:39:26Z

@Hainish Does radix tree look like this:

{
  "com": {
    "example": {
      "^": true,
      "www": { "^": true }
  },
  "net": {
    "example": {
      "^": true,
      "www": { "^": true }
  },
  "org": {
    "eff": {
      "^": true,
      "www": { "^": true }
  },
  "onion": {
    "exampleexampleex": { "^": true }
  }
}

Hainish · 2017-09-05T23:42:41Z

Great, this looks good to me. Merging

cschanaj · 2017-09-06T00:25:10Z

@Hainish please also take a look at #12422 (memory leak) and #12452 (partial information leak) when you have the time. Thank!

cschanaj mentioned this pull request Aug 31, 2017

Fix WebExtension Excessive Memory Usage #12232

Closed

24 tasks

Pasu Chan added 4 commits August 31, 2017 18:10

Update utils/merge-rulesets.py

8665c2b

Update chromium/background.js

d038628

Update utils/merge-rulesets.py

c2ad058

Update chromium/rules.js

3241f01

Update chromium/background.js

2a96f5c

Fix indentation in chromium/rules.js

0f996c1

Update chromium/background.js comments, thanks @koops76

33ca403

ghost reviewed Sep 1, 2017

View reviewed changes

Hainish assigned cowlicks and Hainish Sep 1, 2017

Hainish reviewed Sep 1, 2017

View reviewed changes

Rename parseOneRuleset to parseOneXmlRuleset

f984686

Use for (let ... of) instead of iterations by indices

d437c69

Hainish merged commit 06b50fd into EFForg:master Sep 5, 2017

cschanaj deleted the fix-ruleset-loading branch September 6, 2017 11:25

cowlicks mentioned this pull request Oct 18, 2017

Fix rewriter.js and test the API it uses. #13213

Merged

Uh oh!

Conversation

cschanaj commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 31, 2017

Uh oh!

ghost commented Aug 31, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 31, 2017

Uh oh!

ghost commented Aug 31, 2017

Uh oh!

cschanaj commented Aug 31, 2017

Uh oh!

ghost commented Aug 31, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cschanaj commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 1, 2017

Uh oh!

cschanaj commented Sep 1, 2017

Uh oh!

ghost commented Sep 1, 2017

Uh oh!

ghost commented Sep 1, 2017

Uh oh!

cschanaj commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 1, 2017

Uh oh!

ghost commented Sep 1, 2017

Uh oh!

cschanaj commented Sep 1, 2017

Uh oh!

ghost commented Sep 1, 2017

Uh oh!

ghost commented Sep 1, 2017

Uh oh!

cschanaj commented Sep 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 1, 2017

Uh oh!

Hainish commented Sep 1, 2017

Uh oh!

Hainish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cschanaj commented Aug 31, 2017 •

edited

Loading

ghost commented Aug 31, 2017 •

edited by ghost

Loading

ghost commented Aug 31, 2017 •

edited by ghost

Loading

cschanaj commented Aug 31, 2017 •

edited

Loading

cschanaj commented Sep 1, 2017 •

edited

Loading

ghost commented Sep 2, 2017 •

edited by ghost

Loading

ghost commented Sep 2, 2017 •

edited by ghost

Loading

ghost commented Sep 2, 2017 •

edited by ghost

Loading

cschanaj commented Sep 6, 2017 •

edited

Loading