Issue 2878 - Element Hiding Helper is stuck in a bad state after a tab crash

Issue 2878 - Element Hiding Helper is stuck in a bad state after a tab crash

[Wladimir Palant, 2015-08-10 11:59:24]

Ah, Thomas is on vacation. Manvel, could you take a look maybe?

[saroyanm, 2015-08-10 14:58:13]

https://codereview.adblockplus.org/29323426/diff/29323427/lib/aardvark.js
File lib/aardvark.js (right):

https://codereview.adblockplus.org/29323426/diff/29323427/lib/aardvark.js#new...
lib/aardvark.js:257: return;   // hideSelection() called quit()
Nit: I think make sense to move comment before if statement, for consistency.

https://codereview.adblockplus.org/29323426/diff/29323427/lib/aardvark.js#new...
lib/aardvark.js:400: if (this.boxElem.parentNode)
I wander if this is the only case where we are using CPOW when the tab is
crashed ? 
I've noticed that we also use "this.boxElem.parentNode" in selectElement, have
assumption that tab crash is connected to that CPOW.

https://codereview.adblockplus.org/29323426/diff/29323427/lib/aardvark.js#new...
lib/aardvark.js:406: this.boxElem = {};
Shouldn't we reset this.boxElem value and paintNode in quite() function  ?

[Wladimir Palant, 2015-08-10 21:10:11]

https://codereview.adblockplus.org/29323426/diff/29323427/lib/aardvark.js
File lib/aardvark.js (right):

https://codereview.adblockplus.org/29323426/diff/29323427/lib/aardvark.js#new...
lib/aardvark.js:400: if (this.boxElem.parentNode)
On 2015/08/10 14:58:12, saroyanm wrote:
> I wander if this is the only case where we are using CPOW when the tab is
> crashed ? 

No, it's not, and the other places might still produce errors - but we'll
usually get here (either via onMouseMove or quit). This is merely an
intermediate solution until https://issues.adblockplus.org/ticket/2879 is fixed.

> I've noticed that we also use "this.boxElem.parentNode" in selectElement, have
> assumption that tab crash is connected to that CPOW.

https://codereview.adblockplus.org/29323426/diff/29323427/lib/aardvark.js#new...
lib/aardvark.js:406: this.boxElem = {};
On 2015/08/10 14:58:12, saroyanm wrote:
> Shouldn't we reset this.boxElem value and paintNode in quite() function  ?

No, because quit() will call hideSelection() again - it would normally cause
infinite recursion.

[saroyanm, 2015-08-10 21:33:28]

On 2015/08/10 21:10:11, Wladimir Palant wrote:
> https://codereview.adblockplus.org/29323426/diff/29323427/lib/aardvark.js
> File lib/aardvark.js (right):
> 
>
https://codereview.adblockplus.org/29323426/diff/29323427/lib/aardvark.js#new...
> lib/aardvark.js:400: if (this.boxElem.parentNode)
> On 2015/08/10 14:58:12, saroyanm wrote:
> > I wander if this is the only case where we are using CPOW when the tab is
> > crashed ? 
> 
> No, it's not, and the other places might still produce errors - but we'll
> usually get here (either via onMouseMove or quit). This is merely an
> intermediate solution until https://issues.adblockplus.org/ticket/2879 is
fixed.
> 
> > I've noticed that we also use "this.boxElem.parentNode" in selectElement,
have
> > assumption that tab crash is connected to that CPOW.
> 
>
https://codereview.adblockplus.org/29323426/diff/29323427/lib/aardvark.js#new...
> lib/aardvark.js:406: this.boxElem = {};
> On 2015/08/10 14:58:12, saroyanm wrote:
> > Shouldn't we reset this.boxElem value and paintNode in quite() function  ?
> 
> No, because quit() will call hideSelection() again - it would normally cause
> infinite recursion.

LGTM