Issue 5147 - Invalidate wrapper on delete

Issue 5147 - Invalidate wrapper on delete

[hub, 2017-10-10 23:00:15]

You can test it with the following patch.

````
diff --git a/test/filterClasses.js b/test/filterClasses.js
--- a/test/filterClasses.js
+++ b/test/filterClasses.js
@@ -215,16 +215,17 @@
       test.equal(actualDomains2, expectedDomains, "Correct selector domain for
filter " + expected);

       test.equal(filter1.selector, selector, "Correct selector for filter " +
text);
       test.equal(filter2.selector, selector, "Correct selector for filter " +
expected);
     }

     filter1.delete();
     filter2.delete();
+    test.throws(() => { filter1._pointer == 0xdeadbeef; });
   }

   test.done();
 };

 exports.testSerialize = function(test)
 {
   // Comment
````

[Wladimir Palant, 2017-10-11 07:45:51]

Please add unit tests for this, particularly that calling .delete() twice on the
same object isn't possible even if another reference to this object exists. We
have testGC in test/filterClasses.js, that would the right place I guess.

https://codereview.adblockplus.org/29573044/diff/29573045/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29573044/diff/29573045/compiled/bindings/g...
compiled/bindings/generator.cpp:387: this._pointer = 0xdeadbeef;
This is unnecessary, you are overwriting the property below.

https://codereview.adblockplus.org/29573044/diff/29573045/compiled/bindings/g...
compiled/bindings/generator.cpp:390: throw new Error("Deleted object"); return
0xdeadbeef;
return after throw is dead code.

Nit: "Attemt to use a deleted object" might be a better error message.

[hub, 2017-10-11 12:49:52]

test added, comments addressed.

[sergei, 2017-10-11 12:52:56]

https://codereview.adblockplus.org/29573044/diff/29573705/test/filterClasses.js
File test/filterClasses.js (right):

https://codereview.adblockplus.org/29573044/diff/29573705/test/filterClasses....
test/filterClasses.js:167: test.throws(() => { filter1.delete() == 0; });
IMO it would be better to remove these comparisons.

https://codereview.adblockplus.org/29573044/diff/29573705/test/filterClasses....
test/filterClasses.js:167: test.throws(() => { filter1.delete() == 0; });
What about putting them into a separate test?

[Wladimir Palant, 2017-10-11 13:51:52]

https://codereview.adblockplus.org/29573044/diff/29573705/test/filterClasses.js
File test/filterClasses.js (right):

https://codereview.adblockplus.org/29573044/diff/29573705/test/filterClasses....
test/filterClasses.js:169: test.throws(() => { filter1._pointer == 0xdeadbeef;
});
I agree, comparisons aren't necessary. Also, you should put these before
deleting filter2 - so the instance still exists.

[hub, 2017-10-11 14:47:01]

https://codereview.adblockplus.org/29573044/diff/29573705/test/filterClasses.js
File test/filterClasses.js (right):

https://codereview.adblockplus.org/29573044/diff/29573705/test/filterClasses....
test/filterClasses.js:167: test.throws(() => { filter1.delete() == 0; });
On 2017/10/11 12:52:56, sergei wrote:
> IMO it would be better to remove these comparisons.

I don't know was I put the '== 0' here.

I think they can be in that test. We want to test the object lifecycle and
therefor testing with invalidate them is relevant.

https://codereview.adblockplus.org/29573044/diff/29573705/test/filterClasses....
test/filterClasses.js:169: test.throws(() => { filter1._pointer == 0xdeadbeef;
});
On 2017/10/11 13:51:52, Wladimir Palant wrote:
> I agree, comparisons aren't necessary. Also, you should put these before
> deleting filter2 - so the instance still exists.

Done.

[Wladimir Palant, 2017-10-11 16:39:54]

LGTM