Opened 21 months ago

Closed 20 months ago

Last modified 2 weeks ago

#6385 closed defect (fixed)

[webextension] "response is undefined" exceptions being thrown

Reported by: kzar Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-3.0.3-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: sebastian, mjethani, Ross, agiammarchi Blocked By:
Blocking: Platform: Firefox
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29696583/

Description

Environment

Firefox 58, Adblock Plus devenv built from 8399e34fff05, Debian Buster

How to reproduce

  1. Create a new tab and open the console for it
  2. Browse to https://reddit.com

Observed behaviour

Lots of exceptions showing in the console:

response is undefined  include.preload.js:682

Expected behaviour

Exceptions now being shown.

Notes

This is a regression caused by 8399e34fff05.

When initialising element hiding include.preload.js sends an "elemhide.getSelectors" message and then attempts to access the trace property of the response. However response is sometimes undefined. This happens when updateFrameStyles gets undefined back when attempting to get the frame using ext.getFrame. That doesn't seem to be a bug, framesOfTabs really doesn't have the frame in question, but I'm not sure why that is.

Change History (10)

comment:1 Changed 21 months ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:2 Changed 21 months ago by kzar

  • Cc agiammarchi added

comment:3 follow-up: Changed 20 months ago by mjethani

I dug a little deeper into this. The background page gets onHeadersReceived after it gets the elemhide.getSelectors message from the content script, so it doesn't have the frame at this point. The right thing to do here would be to give up adding of the style sheet at this point entirely (rather than adding it to the DOM) and then add it when we get the onHeadersReceived event. This would involve sending a message to the content script when we've added the style sheet, so it can do its tracing/logging/whatever.

At the very least we could rerun the code for elemhide.getSelectors when we get onHeadersReceived.

comment:4 Changed 20 months ago by abpbot

comment:5 Changed 20 months ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

(I found this workaround was also necessary to get ad blocking to work in Edge at all!)

comment:6 Changed 19 months ago by sebastian

I still keep getting response is undefined include.preload.js:700, right after the extension loads. This might be unrelated though, as it doesn't seem to be a regression. I even get the same with Adblock Plus 3.0. My suspicion is that the content scripts send elemhide.getSelectors messages from the existing tabs before the background page is ready to respond. IIRC, Firefox injects content scripts in existing tabs when loading the extension. For reference, I'm using Firefox 58.

comment:7 Changed 19 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed. The message still occurs after the extension loads as noted above. Opened as #6498.

ABP 3.0.2.1983
Firefox 53 / 58 / Windows 7
Chrome 49 / 65 / Windows 7
Opera 36 / 51 / Windows 7

comment:8 in reply to: ↑ 3 Changed 17 months ago by mjethani

Replying to mjethani:

I dug a little deeper into this. The background page gets onHeadersReceived after it gets the elemhide.getSelectors message from the content script, so it doesn't have the frame at this point. The right thing to do here would be to give up adding of the style sheet at this point entirely (rather than adding it to the DOM) and then add it when we get the onHeadersReceived event. This would involve sending a message to the content script when we've added the style sheet, so it can do its tracing/logging/whatever.

At the very least we could rerun the code for elemhide.getSelectors when we get onHeadersReceived.

Just a thought, but I wonder if #6507 also fixes this properly, because I think the fix we implemented was still kind of a short-term hack and did not address the underlying issue.

comment:9 Changed 5 months ago by Himanshu0709

spam

Last edited 2 weeks ago by kzar (previous) (diff)

comment:10 Changed 5 months ago by goklopp

spam

Last edited 2 weeks ago by kzar (previous) (diff)
Note: See TracTickets for help on using tickets.