Issue 29750566: Issue 6565 - Ensure the URL is known for requests we might block (Closed)
|
Created:
April 12, 2018, 11:51 a.m. by kzar Modified:
April 14, 2018, 3:21 a.m. Reviewers:
Sebastian Noack Visibility:
Public. |
DescriptionIssue 6565 - Ensure the URL is known for requests we might block
Patch Set 1 #
Total comments: 5
MessagesTotal messages: 5
Patch Set 1
https://codereview.adblockplus.org/29750566/diff/29750567/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29750566/diff/29750567/lib/requestBlocker.... lib/requestBlocker.js:154: page = new ext.Page({id: details.tabId, url: details.url}); Shouldn't this be rather the originUrl than the url of this request? But then again I don't get where the error is comming from, since we fall back to the originURL anyway, and the code path that touches the page object in checkWhitelisted() isn't even hit as long as an originUrl is given.
https://codereview.adblockplus.org/29750566/diff/29750567/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29750566/diff/29750567/lib/requestBlocker.... lib/requestBlocker.js:154: page = new ext.Page({id: details.tabId, url: details.url}); On 2018/04/12 13:27:09, Sebastian Noack wrote: > Shouldn't this be rather the originUrl than the url of this request? But then > again I don't get where the error is comming from, since we fall back to the > originURL anyway, and the code path that touches the page object in > checkWhitelisted() isn't even hit as long as an originUrl is given. However, if no originUrl is given these request should be ignored now, by https://issues.adblockplus.org/ticket/6544. Can you still reproduce the error?
https://codereview.adblockplus.org/29750566/diff/29750567/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29750566/diff/29750567/lib/requestBlocker.... lib/requestBlocker.js:154: page = new ext.Page({id: details.tabId, url: details.url}); On 2018/04/12 13:38:09, Sebastian Noack wrote: > On 2018/04/12 13:27:09, Sebastian Noack wrote: > > Shouldn't this be rather the originUrl than the url of this request? But then > > again I don't get where the error is comming from, since we fall back to the > > originURL anyway, and the code path that touches the page object in > > checkWhitelisted() isn't even hit as long as an originUrl is given. > > However, if no originUrl is given these request should be ignored now, by > https://issues.adblockplus.org/ticket/6544. Can you still reproduce the error? Yea, unfortunately I can. https://codereview.adblockplus.org/29750566/diff/29750567/lib/requestBlocker.... lib/requestBlocker.js:154: page = new ext.Page({id: details.tabId, url: details.url}); On 2018/04/12 13:27:09, Sebastian Noack wrote: > Shouldn't this be rather the originUrl than the url of this request? But then > again I don't get where the error is comming from, since we fall back to the > originURL anyway, and the code path that touches the page object in > checkWhitelisted() isn't even hit as long as an originUrl is given. Yea, good point. Well since we now know why you couldn't reproduce this to initially (disabled new tab page) perhaps you could try again to figure out what's happening? I'd rather not change something that I don't fully understand in case I'm missing something important.
Message was sent while issue was closed.
https://codereview.adblockplus.org/29750566/diff/29750567/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29750566/diff/29750567/lib/requestBlocker.... lib/requestBlocker.js:154: page = new ext.Page({id: details.tabId, url: details.url}); On 2018/04/13 15:36:37, kzar wrote: > On 2018/04/12 13:27:09, Sebastian Noack wrote: > > Shouldn't this be rather the originUrl than the url of this request? But then > > again I don't get where the error is comming from, since we fall back to the > > originURL anyway, and the code path that touches the page object in > > checkWhitelisted() isn't even hit as long as an originUrl is given. > > Yea, good point. Well since we now know why you couldn't reproduce this to > initially (disabled new tab page) perhaps you could try again to figure out > what's happening? I'd rather not change something that I don't fully understand > in case I'm missing something important. How do I get that new tab page? 
|
||||||||||||||||||||||||||||||||||||||
