Issue 7236 - Handle sub properties in abort-on-property snippets

test/browser/snippets.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 25 matching lines @@
   snippet(...args);
 
   // For snippets that run in the context of the document via a <script>
   // element (i.e. snippets that use makeInjector()), we need to wait for
   // execution to be complete.
   await timeout(100);
 }
 
 exports.testAbortOnPropertyReadSnippet = async function(test)
 {
-  window.abpTest = "foo";

[Manish Jethani, 2019/03/20 01:42:00]

This seems odd standing out on the top. Let's move this to right above its test
(the first one).

[hub, 2019/03/20 15:10:07]

Done.

-
-  function testProperty(property, result = true, errorType = "ReferenceError")
+  function testProperty(property, result = true, errorName = "ReferenceError")
   {
     let path = property.split(".");
 
     let exceptionCaught = false;
     let value = 1;
 
     try
     {
       let obj = window;
       while (path.length > 1)
         obj = obj[path.shift()];
       value = obj[path.shift()];
     }
     catch (e)
     {
-      test.equal(e.name, errorType);

[Manish Jethani, 2019/03/20 01:42:01]

Nit: Since we're comparing with the `name` property of the `Error` instance,
would be nice to name it `errorName` instead.

[hub, 2019/03/20 15:10:05]

Done.

+      test.equal(e.name, errorName);
       exceptionCaught = true;
     }
 
     test.equal(
       exceptionCaught,
       result,
       `The property "${property}" ${result ? "should" : "shouldn't"} trigger an exception.`
     );
     test.equal(
       value,
       result ? 1 : undefined,
       `The value for "${property}" ${result ? "shouldn't" : "should"} have been read.`
     );
   }
 
+  window.abpTest = "fortytwo";
   await runSnippet(test, "abort-on-property-read", "abpTest");
   testProperty("abpTest");
 
-  window.abpTest2 = {prop1: "foo"};
-

[Manish Jethani, 2019/03/20 01:41:59]

Nit: Blank line here seems unnecessary if we are grouping things together as in
the following tests.

[hub, 2019/03/20 15:10:06]

Done.

+  window.abpTest2 = {prop1: "fortytwo"};
   await runSnippet(test, "abort-on-property-read", "abpTest2.prop1");
   testProperty("abpTest2.prop1");
 
-  // Test that we try to catch a property that don't exist yet.

[Manish Jethani, 2019/03/20 01:41:59]

Nit: s/don't/doesn't/

[hub, 2019/03/20 15:10:06]

Done.

+  // Test that we try to catch a property that doesn't exist yet.
   await runSnippet(test, "abort-on-property-read", "abpTest3.prop1");
-  window.abpTest3 = {prop1: "foo"};
+  window.abpTest3 = {prop1: "fortytwo"};
   testProperty("abpTest3.prop1");
 
   // Test that other properties don't trigger.
   testProperty("abpTest3.prop2", false);
 
-  // Test overwriting the object with another object

[Manish Jethani, 2019/03/20 01:42:01]

Nit: Period at the end (I don't mind too much but consistency would be great).
Also same below.

[hub, 2019/03/20 15:10:06]

Done.

-  window.foo = {bar: {}};

[Manish Jethani, 2019/03/20 01:41:59]

There are some inconsistencies with the naming here. We start with `abpTest`,
then `abpTest2`, `abpTest3`, then suddently switch to `foo`.

I suggest we follow this convention:

 * First-level properties: abpTest, abpTest2, abpTest3, ...
 * Second-level properties: prop1, prop2, prop3, ...
 * Third-level properties: foo, bar, lambda
 * Value: Either 42 (number) or an object

It's more or less like this anyway, just some inconsistencies. You could choose
some other convention if you like, as long as it's consistent and easier for
someone else to recognize and follow the same pattern.

[hub, 2019/03/20 15:10:05]

Done.

-  await runSnippet(test, "abort-on-property-read", "foo.bar.lambda");
-  testProperty("foo.bar.lambda");
-  window.foo.bar = {};
-  testProperty("foo.bar.lambda");
+  // Test overwriting the object with another object.
+  window.abpTest4 = {prop3: {}};
+  await runSnippet(test, "abort-on-property-read", "abpTest4.prop3.foo");
+  testProperty("abpTest4.prop3.foo");
+  window.abpTest4.prop3 = {};
+  testProperty("abpTest4.prop3.foo");
 
-  // Test if we start with a non-object
-  window.foo2 = 5;
-  await runSnippet(test, "abort-on-property-read", "foo2.bar2.lambda");
+  // Test if we start with a non-object.
+  window.abpTest5 = 42;
+  await runSnippet(test, "abort-on-property-read", "abpTest5.prop4.bar");
 
-  // We expect that accessing the property cause a TypeError.

[Manish Jethani, 2019/03/20 01:42:00]

Nit: s/cause/causes/

(FWIW I'm not sure this comment explains anything that isn't obvious from the
code.)

[hub, 2019/03/20 15:10:06]

Deleted.

-  testProperty("foo2.bar2.lambda", true, "TypeError");
+  testProperty("abpTest5.prop4.bar", true, "TypeError");
 
-  window.foo2 = {bar2: {}};

[Manish Jethani, 2019/03/20 01:42:00]

Before this line, we could throw in another test:

  window.foo2 = {bar2: 42};
  testProperty("foo2.bar2.lambda", false);

In this case no error is thrown, but afterwards when replace `bar2` with a `{}`
then in the next access we get an error.

[hub, 2019/03/20 15:10:07]

Done.

-  testProperty("foo2.bar2.lambda");
+  window.abpTest5 = {prop4: 42};
+  testProperty("abpTest5.prop4.bar", false);
+  window.abpTest5 = {prop4: {}};
+  testProperty("abpTest5.prop4.bar");
 
   test.done();
 };