Issue 29362203: Issue 4599 - Update the page structure more eagerly (Closed)
DescriptionIssue 4599 - Update the page structure more eagerly
Patch Set 1 #Patch Set 2 : Improve explanation in comment #
Total comments: 1
Patch Set 3 : Only eagerly update structure for new tabs #Patch Set 4 : Improved comment #
Total comments: 2
MessagesTotal messages: 9
Patch Set 1
Frankly, I don't completely understand why we have that inconsistent state here in the first place. Prerendered tabs (which this seems to be about) have a new tab ID. So theoretically, we should be fine if we just record the sitekey per tab and frame as we receive the headers. If the problem is related to the sitekey given in the HTML, recorded by the content script, we should just remove that logic. We should do so anyway, as we added that logic only for Safari, but for simplicity we didn't bother to skip it on Chrome. Now where this codebase doesn't support Safari anymore, we should remove that logic all together. If this is related to magic done by the PageMap() and/or the fact that we don't associate the sitekey with the corresponding frame ID, but with the frame's URL, we should rather use a simple two-dimensional mapping of tab IDs and frameIDs. We should also do that anyway, now, where we no longer support Safari, and the abstraction layer is matter to go away.
Patch Set 2 : Improve explanation in comment On 2016/11/10 18:12:05, Sebastian Noack wrote: > Frankly, I don't completely understand why we have that inconsistent state here > in the first place. Prerendered tabs (which this seems to be about) have a new > tab ID. So theoretically, we should be fine if we just record the sitekey per > tab and frame as we receive the headers. I did not realise prerendered tabs got a new ID in Chrome, so much of what I assumed and explained in the issue turned out to be wrong. I don't completely understand why the sitekey workaround is still required either, I'm going to look into that now. Hopefully we can remove that hack like you suggested now that Safari support doesn't matter That said this issue is not about sitekeys, but about pages making requests before the onCommitted event was fired. Those requests are being considered as third party since we didn't yet record a URL for the page and therefore don't have a document domain. (Chrome seems to default to chrome-search://local-ntp/local-ntp.html and therefore local-ntp for those on my laptop.) The fix I suggested here makes sense, record the URL earlier and we have the correct URL in time to properly consider those requests as first party.
On 2016/11/11 09:17:13, kzar wrote: > I don't completely understand why the sitekey workaround is still required either, > I'm going to look into that now. Hopefully we can remove that hack like you > suggested now that Safari support doesn't matter (I opened this issue for the sitekey stuff: https://issues.adblockplus.org/ticket/4632 )
NOT LGTM https://codereview.adblockplus.org/29362203/diff/29362445/chrome/ext/backgrou... File chrome/ext/background.js (right): https://codereview.adblockplus.org/29362203/diff/29362445/chrome/ext/backgrou... chrome/ext/background.js:176: details.url, false, details.parentFrameId); You are essentially reverting changes from issue 4386, the onCommitted handler no longer serves any purpose. This is bad: onBeforeNavigate is often called when no navigation occurs in the end. In this scenario you will end up misattributing all requests from a tab to a page that never loaded.
Patch Set 3 : Only eagerly update structure for new tabs Well how about this? Only eagerly updating the structure for new tabs.
Patch Set 4 : Improved comment
https://codereview.adblockplus.org/29362203/diff/29362520/chrome/ext/backgrou... File chrome/ext/background.js (right): https://codereview.adblockplus.org/29362203/diff/29362520/chrome/ext/backgrou... chrome/ext/background.js:176: if (!(details.tabId in framesOfTabs)) While this is probably not wrong, I think that you are only solving a small part of the issue with this approach...
https://codereview.adblockplus.org/29362203/diff/29362520/chrome/ext/backgrou... File chrome/ext/background.js (right): https://codereview.adblockplus.org/29362203/diff/29362520/chrome/ext/backgrou... chrome/ext/background.js:176: if (!(details.tabId in framesOfTabs)) On 2016/11/14 15:56:29, Wladimir Palant wrote: > While this is probably not wrong, I think that you are only solving a > small part of the issue with this approach... Well so far the issue only described a problem with pre-rendered tabs, but I have just tested and confirmed you're indeed right that the problem is not limited to them. When clicking a link to http://www.apptap.com I saw requests being made just before its onComitted event fired. Back to the drawing board... 
|
