Issue 29482663: Issue 5386 - Ignore messages from content scripts in devtools panel (Closed)
|
Created:
July 7, 2017, 3:11 p.m. by Manish Jethani Modified:
Aug. 16, 2017, 10:47 a.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 5386 - Ignore messages from content scripts in devtools panel
Patch Set 1 #Patch Set 2 : Check for chrome.devtools instead #
Total comments: 6
Patch Set 3 : Use consistent style for checking for availability of API #
Total comments: 3
Patch Set 4 : Use includes instead of indexOf #MessagesTotal messages: 16
Patch Set 1 devtools.js sets ext.onMessage to port.onMessage In Firefox 55, the browser is sending messages from the content script to the devtools panel as well. Since there is no _dispatch function there, it throws an error, which causes the subsequent listener in the background page to return undefined in response to "get-selectors" (even though the listener itself is actually successfully returning the selectors). The fix is to ignore any messages coming from the content script itself.
On 2017/07/07 15:16:45, Manish Jethani wrote: > The fix is to ignore any messages coming from the content script itself. By the way, I'm not sure if this break anything in the content script. Looking for your inputs. We could alternatively restrict this check to the devtools panel only.
I agree to this solution. If the message is sent by a content script sender.tab is set accordingly. On Chrome, in this case, the onMessage listener isn't called in other content scripts. So ignoring messages in content scripts if sender.tab is set should give the same behavior on Firefox. However, can you please add a comment that this is a workaround for Firefox, referring to the respective bug? If there is none, we should file one.
Patch Set 2: Check for chrome.devtools instead I've modified the fix a bit. Even if sender.tab weren't set, the devtools panel wouldn't know what to do with the message (it would still crash), since ext.onMessage doesn't have a _dispatch property here. The right thing to do is to not listen for messages here if this is the devtools panel. No messages are expected, as the only way for the background page communicate with the devtools panel is over the port opened in ext/devtools.js. I've also added a comment pointing to the Firefox bug. https://codereview.adblockplus.org/29482663/diff/29497558/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29482663/diff/29497558/ext/content.js#newc... ext/content.js:9: // Listen for messages from the background page. I've added this extra comment here to clarify what we're really listening for here, since it wasn't obvious when I first looked at the code.
https://codereview.adblockplus.org/29482663/diff/29497558/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29482663/diff/29497558/ext/content.js#newc... ext/content.js:14: }); I might be missing something here, but this way the devtools panel won't listen to any messages whatsoever, right? Effectively, it's the same as not loading this file in devtools-panel.html at all. But the devtools panel is actually listening to messages from the background page like "add-record", it merely doesn't want to receive messages from content scripts.
https://codereview.adblockplus.org/29482663/diff/29497558/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29482663/diff/29497558/ext/content.js#newc... ext/content.js:14: }); On 2017/08/16 08:56:44, Wladimir Palant wrote: > I might be missing something here, but this way the devtools panel won't listen > to any messages whatsoever, right? Effectively, it's the same as not loading > this file in devtools-panel.html at all. But the devtools panel is actually > listening to messages from the background page like "add-record", it merely > doesn't want to receive messages from content scripts. Yeah, so the devtools panel opens a persistent connection to the background page (rather than listening for one-off messages) [1], and messages like "add-record" come over that persistent connection. It's the only way for the devtools panel to really communicate with the background page. You can see that ext.onMessage is getting reassigned in ext/devtools.js If it wasn't for this bug in Firefox, the runtime.onMessage handler would never get called. [1]: https://hg.adblockplus.org/adblockpluschrome/file/552ac1922795/ext/devtools.j...
Patch Set 3: Use consistent style for checking for availability of API
https://codereview.adblockplus.org/29482663/diff/29497558/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29482663/diff/29497558/ext/content.js#newc... ext/content.js:14: }); On 2017/08/16 09:18:54, Manish Jethani wrote: > You can see that ext.onMessage is getting reassigned in ext/devtools.js I see. So why is devtools-panel.html loading ext/content.js in the first place? It seems that not loading it will fix this issue as well.
https://codereview.adblockplus.org/29482663/diff/29497558/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29482663/diff/29497558/ext/content.js#newc... ext/content.js:14: }); On 2017/08/16 09:28:33, Wladimir Palant wrote: > On 2017/08/16 09:18:54, Manish Jethani wrote: > > You can see that ext.onMessage is getting reassigned in ext/devtools.js > > I see. So why is devtools-panel.html loading ext/content.js in the first place? > It seems that not loading it will fix this issue as well. I went down that route [1] but decided to back off because it would need more changes, and it didn't seem worth the effort at the time. Mainly the test environment for the devtools panel also needs ext/content.js, but the one in adblockplusui instead [2], which initializes the background page in an iframe and so on and sets up the messaging. Ideally it would be fixed that way, along with any other inconsistencies found. If you think it's worth going down that route, I can give it a shot. [1]: https://codereview.adblockplus.org/29494581/ [2]: https://hg.adblockplus.org/adblockplusui/file/461ee88245c7/ext/content.js
LGTM https://codereview.adblockplus.org/29482663/diff/29497558/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29482663/diff/29497558/ext/content.js#newc... ext/content.js:14: }); On 2017/08/16 09:42:43, Manish Jethani wrote: > I went down that route [1] but decided to back off because it would need more > changes, and it didn't seem worth the effort at the time. Mainly the test > environment for the devtools panel also needs ext/content.js, but the one in > adblockplusui instead [2], which initializes the background page in an iframe > and so on and sets up the messaging. You are right, doesn't seem to be worth it right now.
https://codereview.adblockplus.org/29482663/diff/29517559/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29482663/diff/29517559/ext/content.js#newc... ext/content.js:12: return ext.onMessage._dispatch(message, {}, sendResponse).indexOf(true) != While changing this code anyway, you might want to replace `.indexOf(true) != -1` with `.contains(true)`. Then you also wouldn't need to wrap this line.
Patch Set 4: Use includes instead of indexOf https://codereview.adblockplus.org/29482663/diff/29517559/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29482663/diff/29517559/ext/content.js#newc... ext/content.js:12: return ext.onMessage._dispatch(message, {}, sendResponse).indexOf(true) != On 2017/08/16 10:15:00, Sebastian Noack wrote: > While changing this code anyway, you might want to replace `.indexOf(true) != > -1` with `.contains(true)`. Then you also wouldn't need to wrap this line. I'm assuming you meant `.includes(true)` Done.
LGTM https://codereview.adblockplus.org/29482663/diff/29517559/ext/content.js File ext/content.js (right): https://codereview.adblockplus.org/29482663/diff/29517559/ext/content.js#newc... ext/content.js:12: return ext.onMessage._dispatch(message, {}, sendResponse).indexOf(true) != On 2017/08/16 10:30:36, Manish Jethani wrote: > On 2017/08/16 10:15:00, Sebastian Noack wrote: > > While changing this code anyway, you might want to replace `.indexOf(true) != > > -1` with `.contains(true)`. Then you also wouldn't need to wrap this line. > > I'm assuming you meant `.includes(true)` > > Done. Sure, that is what I meant, sorry.
LGTM
|
