Issue 242 - Do not disable user style sheets on unrelated errors

[Sebastian Noack, 2018-02-07 15:05:02]

Can you elaborate why this check is necessary?

[Manish Jethani, 2018-02-07 15:05:39]

Patch Set 1

I noticed this on one website: a frame gets removed from the document between
the time the content scripts sends the elemhide.getSelectors message and the
time the background page tries to inject a user style sheet, which causes
tabs.insertCSS to throw a frame-not-found error. There may also be other reasons
for failure that have nothing to do with cssOrigin support. It's best to check
for "cssOrigin" in the error message specifically.

[Manish Jethani, 2018-02-07 15:07:05]

On 2018/02/07 15:05:39, Manish Jethani wrote:
> Patch Set 1
> 
> I noticed this on one website: a frame gets removed from the document between
> the time the content scripts sends the elemhide.getSelectors message and the
> time the background page tries to inject a user style sheet, which causes
> tabs.insertCSS to throw a frame-not-found error. There may also be other
reasons
> for failure that have nothing to do with cssOrigin support. It's best to check
> for "cssOrigin" in the error message specifically.

To be clear, because of that one frame in one document the background page never
tries to use tabs.insertCSS again. This should not happen.

[Sebastian Noack, 2018-02-07 15:15:40]

On 2018/02/07 15:07:05, Manish Jethani wrote:
> On 2018/02/07 15:05:39, Manish Jethani wrote:
> > Patch Set 1
> > 
> > I noticed this on one website: a frame gets removed from the document
between
> > the time the content scripts sends the elemhide.getSelectors message and the
> > time the background page tries to inject a user style sheet, which causes
> > tabs.insertCSS to throw a frame-not-found error. There may also be other
> reasons
> > for failure that have nothing to do with cssOrigin support. It's best to
check
> > for "cssOrigin" in the error message specifically.
> 
> To be clear, because of that one frame in one document the background page
never
> tries to use tabs.insertCSS again. This should not happen.

I wonder whether re-throwing the exception is the right thing to do in this
scenario. If the frame does no longer exist, this seems to be expected in such a
race condition. So do we really want to spam the error log in that scenario?

[Manish Jethani, 2018-02-07 15:21:28]

On 2018/02/07 15:15:40, Sebastian Noack wrote:
> On 2018/02/07 15:07:05, Manish Jethani wrote:
> > On 2018/02/07 15:05:39, Manish Jethani wrote:
> > > Patch Set 1
> > > 
> > > I noticed this on one website: a frame gets removed from the document
> between
> > > the time the content scripts sends the elemhide.getSelectors message and
the
> > > time the background page tries to inject a user style sheet, which causes
> > > tabs.insertCSS to throw a frame-not-found error. There may also be other
> > reasons
> > > for failure that have nothing to do with cssOrigin support. It's best to
> check
> > > for "cssOrigin" in the error message specifically.
> > 
> > To be clear, because of that one frame in one document the background page
> never
> > tries to use tabs.insertCSS again. This should not happen.
> 
> I wonder whether re-throwing the exception is the right thing to do in this
> scenario. If the frame does no longer exist, this seems to be expected in such
a
> race condition. So do we really want to spam the error log in that scenario?

I'm OK with returning false here without setting userStyleSheetsSupported to
false, but then we'll lose any and all errors. I noticed this error only because
I was looking specifically, otherwise it just switched to inline style sheets
silently. It's a tradeoff. I think maybe at this point it's OK to throw the
error so we can learn about all the situations in which the call fails.

[Manish Jethani, 2018-02-20 11:18:53]

On 2018/02/07 15:21:28, Manish Jethani wrote:
> On 2018/02/07 15:15:40, Sebastian Noack wrote:
> > On 2018/02/07 15:07:05, Manish Jethani wrote:
> > > On 2018/02/07 15:05:39, Manish Jethani wrote:
> > > > Patch Set 1
> > > > 
> > > > I noticed this on one website: a frame gets removed from the document
> > between
> > > > the time the content scripts sends the elemhide.getSelectors message and
> the
> > > > time the background page tries to inject a user style sheet, which
causes
> > > > tabs.insertCSS to throw a frame-not-found error. There may also be other
> > > reasons
> > > > for failure that have nothing to do with cssOrigin support. It's best to
> > check
> > > > for "cssOrigin" in the error message specifically.
> > > 
> > > To be clear, because of that one frame in one document the background page
> > never
> > > tries to use tabs.insertCSS again. This should not happen.
> > 
> > I wonder whether re-throwing the exception is the right thing to do in this
> > scenario. If the frame does no longer exist, this seems to be expected in
such
> a
> > race condition. So do we really want to spam the error log in that scenario?
> 
> I'm OK with returning false here without setting userStyleSheetsSupported to
> false, but then we'll lose any and all errors. I noticed this error only
because
> I was looking specifically, otherwise it just switched to inline style sheets
> silently. It's a tradeoff. I think maybe at this point it's OK to throw the
> error so we can learn about all the situations in which the call fails.

Any thoughts on this, Sebastian? I'm OK with doing it the way you suggest here.

[Manish Jethani, 2018-02-21 15:19:56]

Patch Set 2: Catch all errors but mark userStyleSheetsSupported=false only for
cssOrigin error

[kzar, 2018-02-26 15:25:03]

LGTM