Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(7)

Issue 29691627: Issue 242 - Catch only cssOrigin error from tabs.insertCSS

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 4 days ago by Manish Jethani
Modified:
1 week, 4 days ago
Reviewers:
Sebastian Noack, kzar
Base URL:
https://hg.adblockplus.org/adblockpluschrome/
Visibility:
Public.

Description

Issue 242 - Catch only cssOrigin error from tabs.insertCSS

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -0 lines) Patch
M lib/cssInjection.js View 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 6
Manish Jethani
1 week, 4 days ago (2018-02-07 15:01:03 UTC) #1
Sebastian Noack
Can you elaborate why this check is necessary?
1 week, 4 days ago (2018-02-07 15:05:02 UTC) #2
Manish Jethani
Patch Set 1 I noticed this on one website: a frame gets removed from the ...
1 week, 4 days ago (2018-02-07 15:05:39 UTC) #3
Manish Jethani
On 2018/02/07 15:05:39, Manish Jethani wrote: > Patch Set 1 > > I noticed this ...
1 week, 4 days ago (2018-02-07 15:07:05 UTC) #4
Sebastian Noack
On 2018/02/07 15:07:05, Manish Jethani wrote: > On 2018/02/07 15:05:39, Manish Jethani wrote: > > ...
1 week, 4 days ago (2018-02-07 15:15:40 UTC) #5
Manish Jethani
1 week, 4 days ago (2018-02-07 15:21:28 UTC) #6
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5