-
Notifications
You must be signed in to change notification settings - Fork 241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolve extension startup errors #802
Conversation
ee66cdc
to
9c064d8
Compare
9c064d8
to
eb4672f
Compare
eb4672f
to
dadf073
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this wrapping and guarding of these functions is the correct approach. As a consumer of that API I don't know if a result is correct, or just because loading isn't finish. We should instead use a hasTrackerListLoaded
on the features, e.g. in the webRequest listeners return early when the list isn't loaded yet.
@@ -629,7 +629,7 @@ chrome.runtime.onMessage.addListener((req, sender, res) => { | |||
let sessionKey = getHash() | |||
|
|||
function getArgumentsObject (tabId, sender, documentUrl) { | |||
const tab = tabManager.get({ tabId }) | |||
const tab = tabManager.createOrUpdateTab(tabId, sender.tab) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we write down which cases this is for? Is there a case when a content-script message comes before chrome.tabs.onUpdated
fires? Or is this just for the case when the extension is loaded/reload and content-scripts trigger for existing tabs (in which case we could do chrome.tabs.query
on startup)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or is this just for the case when the extension is loaded/reload and content-scripts trigger for existing tabs (in which case we could do chrome.tabs.query on startup)?
This is the case I was testing. I will check.
let tracker = trackers.getTrackerData(requestData.url, thisTab.site.url, requestData) | ||
let tracker = trackerutils.getTrackerData(requestData.url, thisTab.site.url, requestData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we make this change upstream, or extend from Trackers
in trackers.es6.js
? I think there is a lot of cases where we are using trackers
directly, would we have to update every one to trackerutils
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I was trying not to modify this code given the short timeline however I think we should as you mention do it upstream and propagate it back.
Arguably the consumer here is in control of tracking when they've pushed data in. However I'm happy to update the producer to fail gracefully and expect the consumer to check a method that they expose. I'm going to close this as we picked another avenue for now. I'll create a new issue to track this so we can get it resolved properly. |
You could also delay registering the listeners until completion of all async tasks required by the listeners. Here is a related extension/browser startup issue, by the way: EFForg/privacybadger#1845. |
Reviewer:
Description:
Fixes issue with tracker methods that check the list without knowing if the list has loaded.
Errors such as:
This makes all tracker checks fail open which I think is essentially what we are doing currently whilst also breaking other code.
This also handles missing tab data for
![image](https://user-images.githubusercontent.com/338988/129201372-3b731f12-3ceb-4689-9e65-58c85eb38d10.png)
getArgumentsObject
calls by creating a tab given the sender info. This only happens on extension startup time:This also handles getSettings calls not returning as they've not loaded yet:
![image](https://user-images.githubusercontent.com/338988/129201348-d145a4e0-4040-41a0-b5f7-bfbc5ef78c23.png)
Steps to test this PR:
Automated tests:
Reviewer Checklist:
PR Author Checklist: