Issue 6385 - Don't attempt to inject styles for frames we can't find

Issue 6385 - Don't attempt to inject styles for frames we can't find

[kzar, 2018-02-13 15:00:38]

Patch Set 1

[Manish Jethani, 2018-02-14 13:41:17]

https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js
File lib/cssInjection.js (right):

https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js...
lib/cssInjection.js:110: return;
OK, I guess this is the right thing to do. But we should return false
explicitly.

[Manish Jethani, 2018-02-14 14:03:41]

https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js
File lib/cssInjection.js (right):

https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js...
lib/cssInjection.js:110: return;
On 2018/02/14 13:41:16, Manish Jethani wrote:
> OK, I guess this is the right thing to do. But we should return false
> explicitly.

If this is reproduceable, we should check why it is happening. Clearly the
content script is still around when it gets a response, then we should find out
why ext.getFrame doesn't find the frame.

[kzar, 2018-02-14 14:52:58]

Patch Set 2 : Return `false` explicitly instead of `undefined` implicitly

https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js
File lib/cssInjection.js (right):

https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js...
lib/cssInjection.js:110: return;
On 2018/02/14 13:41:16, Manish Jethani wrote:
> OK, I guess this is the right thing to do. But we should return false
> explicitly.

Done.

https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js...
lib/cssInjection.js:110: return;
On 2018/02/14 14:03:41, Manish Jethani wrote:
> On 2018/02/14 13:41:16, Manish Jethani wrote:
> > OK, I guess this is the right thing to do. But we should return false
> > explicitly.
> 
> If this is reproduceable, we should check why it is happening. Clearly the
> content script is still around when it gets a response, then we should find
out
> why ext.getFrame doesn't find the frame.

This is reproducible, the issue description is up to date with steps of how to
reproduce and also my findings (tl;dr `framesOfTabs` really doesn't have the
frame in question - I'm not sure why).

[Manish Jethani, 2018-02-15 14:06:22]

https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js
File lib/cssInjection.js (right):

https://codereview.adblockplus.org/29696583/diff/29696584/lib/cssInjection.js...
lib/cssInjection.js:110: return;
On 2018/02/14 14:52:58, kzar wrote:
> On 2018/02/14 14:03:41, Manish Jethani wrote:
> > On 2018/02/14 13:41:16, Manish Jethani wrote:
> > > OK, I guess this is the right thing to do. But we should return false
> > > explicitly.
> > 
> > If this is reproduceable, we should check why it is happening. Clearly the
> > content script is still around when it gets a response, then we should find
> out
> > why ext.getFrame doesn't find the frame.
> 
> This is reproducible, the issue description is up to date with steps of how to
> reproduce and also my findings (tl;dr `framesOfTabs` really doesn't have the
> frame in question - I'm not sure why).

See my comment here:

https://issues.adblockplus.org/ticket/6385#comment:3

So I think this is OK as a short-term fix, but I think this is going to happen
very often on Firefox so we should try to do something better.

LGTM