Issue 29720585: Noissue - Explicitly ignore asynchronous errors from tabs.insertCSS (Closed)
|
Created:
March 12, 2018, 10:45 a.m. by Manish Jethani Modified:
March 13, 2018, 11:21 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add comments #Patch Set 3 : Update comment #
Total comments: 2
Patch Set 4 : Update comment #
Total comments: 6
MessagesTotal messages: 14
Patch Set 1
This is somewhat related to changeset 610f216e7442
On Canary it seems the error about the non-existent frame is now asynchronous,
so that it comes as a promise rejection. That's OK but it clogs the log
unnecessarily. Since we have decided to ignore the error, we should also handle
the promise rejection with an empty handler.
The entry in the log looks like this:
Uncaught (in promise) Error: No frame with id 574 in tab 266.
at Object.value [as insertCSS] (polyfill.js:89)
at addStyleSheet (cssInjection.js:73)
at updateFrameStyles (cssInjection.js:127)
at port.on (cssInjection.js:164)
at Port._onMessage (messaging.js:44)
at ext._EventTarget._dispatch (common.js:41)
at browser.runtime.onMessage.addListener (background.js:663)
at wrapper (polyfill.js:153)
at EventImpl.dispatchToListener (extensions::event_bindings:403)
at Event.publicClassPrototype.(anonymous function) [as dispatchToListener]
(extensions::utils:138:26)
https://codereview.adblockplus.org/29720585/diff/29720586/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29720585/diff/29720586/lib/cssInjection.js... lib/cssInjection.js:80: .catch(() => {}); To clarify, so the catch-block below is essentially only hit on Firefox, and the catch-callback is essentially only called on Chromium? Why is it necessary to filter for error messages that contain "cssOrigin" on Firefox, but not on Chromium?
https://codereview.adblockplus.org/29720585/diff/29720586/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29720585/diff/29720586/lib/cssInjection.js... lib/cssInjection.js:80: .catch(() => {}); On 2018/03/12 22:41:25, Sebastian Noack wrote: > To clarify, so the catch-block below is essentially only hit on Firefox, and the > catch-callback is essentially only called on Chromium? > > Why is it necessary to filter for error messages that contain "cssOrigin" on > Firefox, but not on Chromium? On both Firefox and Chrome 65 the error for "cssOrigin" is synchronous, so we're handling that correctly for both platforms. Additionally, on Chrome 66+ I am getting the error for "frame not found" (see stack trace) asynchronously. This is the one that clogs up the log. We won't be getting the "cssOrigin" error asynchronously because Chrome 66+ doesn't throw that error.
https://codereview.adblockplus.org/29720585/diff/29720586/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29720585/diff/29720586/lib/cssInjection.js... lib/cssInjection.js:80: .catch(() => {}); On 2018/03/13 06:09:47, Manish Jethani wrote: > On 2018/03/12 22:41:25, Sebastian Noack wrote: > > To clarify, so the catch-block below is essentially only hit on Firefox, and > the > > catch-callback is essentially only called on Chromium? > > > > Why is it necessary to filter for error messages that contain "cssOrigin" on > > Firefox, but not on Chromium? > > On both Firefox and Chrome 65 the error for "cssOrigin" is synchronous, so we're > handling that correctly for both platforms. Additionally, on Chrome 66+ I am > getting the error for "frame not found" (see stack trace) asynchronously. This > is the one that clogs up the log. > > We won't be getting the "cssOrigin" error asynchronously because Chrome 66+ > doesn't throw that error. I see. Perhaps add some comments here, clarifying when we get an asynchronous error, and when insertCSS() throws synchronously.
https://codereview.adblockplus.org/29720585/diff/29720586/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29720585/diff/29720586/lib/cssInjection.js... lib/cssInjection.js:80: .catch(() => {}); On 2018/03/13 15:38:45, Sebastian Noack wrote: > On 2018/03/13 06:09:47, Manish Jethani wrote: > > On 2018/03/12 22:41:25, Sebastian Noack wrote: > > > To clarify, so the catch-block below is essentially only hit on Firefox, and > > the > > > catch-callback is essentially only called on Chromium? > > > > > > Why is it necessary to filter for error messages that contain "cssOrigin" on > > > Firefox, but not on Chromium? > > > > On both Firefox and Chrome 65 the error for "cssOrigin" is synchronous, so > we're > > handling that correctly for both platforms. Additionally, on Chrome 66+ I am > > getting the error for "frame not found" (see stack trace) asynchronously. This > > is the one that clogs up the log. > > > > We won't be getting the "cssOrigin" error asynchronously because Chrome 66+ > > doesn't throw that error. > > I see. Perhaps add some comments here, clarifying when we get an asynchronous > error, and when insertCSS() throws synchronously. Done.
Patch Set 2: Add comments
Patch Set 3: Update comment
https://codereview.adblockplus.org/29720585/diff/29721731/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29720585/diff/29721731/lib/cssInjection.js... lib/cssInjection.js:98: // errors. This is only the case on Firefox, while Chromium gives an asynchronous error in this case, right? Mind making this explicit in the comment here? Now, I also realize, that this means in case the frame doesn't exist anymore, on Chrome we return true, but on Firefox we return false?
Patch Set 4: Update comment https://codereview.adblockplus.org/29720585/diff/29721731/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29720585/diff/29721731/lib/cssInjection.js... lib/cssInjection.js:98: // errors. On 2018/03/13 18:40:58, Sebastian Noack wrote: > This is only the case on Firefox, while Chromium gives an asynchronous error in > this case, right? Mind making this explicit in the comment here? > > Now, I also realize, that this means in case the frame doesn't exist anymore, on > Chrome we return true, but on Firefox we return false? Done. https://codereview.adblockplus.org/29720585/diff/29721797/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29720585/diff/29721797/lib/cssInjection.js... lib/cssInjection.js:82: promise.catch(() => {}); I actually find it a lot cleaner to have a promise variable here, it lets us make this a separate JS statement and attach a comment to it.
https://codereview.adblockplus.org/29720585/diff/29721797/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29720585/diff/29721797/lib/cssInjection.js... lib/cssInjection.js:82: promise.catch(() => {}); On 2018/03/13 22:08:38, Manish Jethani wrote: > I actually find it a lot cleaner to have a promise variable here, it lets us > make this a separate JS statement and attach a comment to it. Well, now where you you just refer to the other comment, you could as well just use an end-of-line comment here: browser.tabs.insertCSS(tabId, { ... }).catch(() => {}); // see below https://codereview.adblockplus.org/29720585/diff/29721797/lib/cssInjection.js... lib/cssInjection.js:104: // the latter case, we cannot indicate failure to the caller, but we still Well, we could signal failure, by returning a promise from this function. But this seems to make the calling code somewhat complex. However, I'm unsure what the consequence of ignoring failure on Chrome is, here.
https://codereview.adblockplus.org/29720585/diff/29721797/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29720585/diff/29721797/lib/cssInjection.js... lib/cssInjection.js:82: promise.catch(() => {}); On 2018/03/13 23:02:21, Sebastian Noack wrote: > On 2018/03/13 22:08:38, Manish Jethani wrote: > > I actually find it a lot cleaner to have a promise variable here, it lets us > > make this a separate JS statement and attach a comment to it. > > Well, now where you you just refer to the other comment, you could as well just > use an end-of-line comment here: > > browser.tabs.insertCSS(tabId, { > ... > }).catch(() => {}); // see below Ack, but I do prefer this if you don't mind. https://codereview.adblockplus.org/29720585/diff/29721797/lib/cssInjection.js... lib/cssInjection.js:104: // the latter case, we cannot indicate failure to the caller, but we still On 2018/03/13 23:02:21, Sebastian Noack wrote: > Well, we could signal failure, by returning a promise from this function. But That's not necessary at least for the case of the frame gone missing. > this seems to make the calling code somewhat complex. However, I'm unsure what > the consequence of ignoring failure on Chrome is, here. The consequence in the worst case should be that no style sheet is injected anywhere. I think perhaps we should only ignore the specific error about missing frames here and let any other errors get logged to the console, but in any case the consequence isn't too bad.
https://codereview.adblockplus.org/29720585/diff/29721797/lib/cssInjection.js File lib/cssInjection.js (right): https://codereview.adblockplus.org/29720585/diff/29721797/lib/cssInjection.js... lib/cssInjection.js:104: // the latter case, we cannot indicate failure to the caller, but we still On 2018/03/13 23:08:30, Manish Jethani wrote: > On 2018/03/13 23:02:21, Sebastian Noack wrote: > > Well, we could signal failure, by returning a promise from this function. But > > That's not necessary at least for the case of the frame gone missing. Also note that returning a promise from here would really complicate the implementation of updateFrameStyles. We should avoid that.
LGTM 
|
