Issue 29958567: Issue 7104 - Clear style sheets when frame structure is updated (Closed)
|
Created:
Dec. 4, 2018, 2:33 p.m. by Manish Jethani Modified:
Dec. 19, 2018, 2:02 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
Patch Set 1 #Patch Set 2 : Use state object (includes rebase) #
Total comments: 9
Patch Set 3 : Use Object.create #Patch Set 4 : Update comment #MessagesTotal messages: 18
I suspect this issue is a symptom of a problem in our page/frame logic, or a Firefox bug which we'll have to work around in that logic. Your suggested workaround was extremely useful since it illustrated that, but I'd rather instead prevent the frame Object from being reused in the first place. Any objection if I take this one over?
On 2018/12/07 13:31:08, kzar wrote: > I suspect this issue is a symptom of a problem in our page/frame logic, or a > Firefox bug which we'll have to work around in that logic. > > Your suggested workaround was extremely useful since it illustrated that, but > I'd rather instead prevent the frame Object from being reused in the first > place. Any objection if I take this one over? Actually, I expected the frame Object to be created fresh each time, but now that I look at the code that's not the intention. Scratch that :/
I wonder, is there a way to do this without putting content filtering logic in the ext code? I'd like to avoid that. I wonder if creating a new Object for a frame every time `createFrame()` was called would be enough, or if that would break some other code which assumed the Object would be preserved. I had a look through the Git history, but couldn't see any obvious reasons why we preserved frame Objects, what do you think Sebastian? Also, I noticed this comment in lib/contentFiltering.js: > Ideally we would compare the old and new style sheets and skip this code > if they're the same, but the old style sheet can be a leftover from a > previous instance of the frame. We must add the new style sheet > regardless. From the sounds of it that means we can simplify that logic while we're at it, since we'll know the stylesheet isn't a leftover now.
On 2018/12/07 13:35:40, kzar wrote: > On 2018/12/07 13:31:08, kzar wrote: > > I suspect this issue is a symptom of a problem in our page/frame logic, or a > > Firefox bug which we'll have to work around in that logic. > > > > Your suggested workaround was extremely useful since it illustrated that, but > > I'd rather instead prevent the frame Object from being reused in the first > > place. Any objection if I take this one over? > > Actually, I expected the frame Object to be created fresh each time, but now > that I look at the code that's not the intention. Scratch that :/ Yeah, I thought this was a bug or a recent change in behavior in the browser, but actually we're reusing the frame object.
On 2018/12/07 14:08:51, kzar wrote:
> I wonder, is there a way to do this without putting content filtering logic in
> the ext code?
We could let the frame object have a generic object containing arbitrary
properties.
if (!frame)
{
frame = {};
frames.set(frameId, frame);
}
frame.state = {};
createFrame() would clear the state (while leaving everything else as it is). In
lib/contentFiltering.js we would then add the style sheets to the state.
if (!frame.state.injectedStyleSheets)
frame.state.injectedStyleSheets = new Map();
I think creating new objects may not work because they have references to each
other (esp. frame.parent) and this code is run multiple times during load, but I
may be wrong.
Hmm, perhaps we should just give frames a `state` Object which is cleared by `createFrame` in ext/background.js. Then other code, e.g. the content filtering code, is free to store frame state there?
On 2018/12/10 13:27:57, kzar wrote: > Hmm, perhaps we should just give frames a `state` Object which is cleared by > `createFrame` in ext/background.js. Then other code, e.g. the content filtering > code, is free to store frame state there? Yes, that's essentially what I meant.
On 2018/12/11 07:55:02, Manish Jethani wrote: > On 2018/12/10 13:27:57, kzar wrote: > > Hmm, perhaps we should just give frames a `state` Object which is cleared by > > `createFrame` in ext/background.js. Then other code, e.g. the content > filtering > > code, is free to store frame state there? > > Yes, that's essentially what I meant. Cool, well want to update the review to do that?
On 2018/12/18 11:38:35, kzar wrote: > On 2018/12/11 07:55:02, Manish Jethani wrote: > > On 2018/12/10 13:27:57, kzar wrote: > > > Hmm, perhaps we should just give frames a `state` Object which is cleared by > > > `createFrame` in ext/background.js. Then other code, e.g. the content > > filtering > > > code, is free to store frame state there? > > > > Yes, that's essentially what I meant. > > Cool, well want to update the review to do that? Done. Patch Set 2: Use state object (Please ignore the inter-patch diff, I have rebased it.)
https://codereview.adblockplus.org/29958567/diff/29964555/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29958567/diff/29964555/ext/background.js#n... ext/background.js:136: frame.state = {}; Nit: How about `Object.create(null)`? https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... File lib/contentFiltering.js (left): https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... lib/contentFiltering.js:116: // Ideally we would compare the old and new style sheets and skip this code This comment implies we can simplify the logic here now that we know the injectedStyleSheets for a frame will be cleared?
https://codereview.adblockplus.org/29958567/diff/29964555/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29958567/diff/29964555/ext/background.js#n... ext/background.js:136: frame.state = {}; On 2018/12/19 10:22:00, kzar wrote: > Nit: How about `Object.create(null)`? Done. https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... File lib/contentFiltering.js (left): https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... lib/contentFiltering.js:116: // Ideally we would compare the old and new style sheets and skip this code On 2018/12/19 10:22:00, kzar wrote: > This comment implies we can simplify the logic here now that we know the > injectedStyleSheets for a frame will be cleared? I'd rather not touch this part now unless I'm sure that it works for all cases (data: URI frames, about:srcdoc frames, etc.). We can do one of two things: (1) go through the effort of testing of that as part of this change; or (2) fix the bug that has been reported with just these changes, then open another ticket for further work to simplify this code. What do you think?
https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... File lib/contentFiltering.js (left): https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... lib/contentFiltering.js:116: // Ideally we would compare the old and new style sheets and skip this code On 2018/12/19 13:03:54, Manish Jethani wrote: > On 2018/12/19 10:22:00, kzar wrote: > > This comment implies we can simplify the logic here now that we know the > > injectedStyleSheets for a frame will be cleared? > > I'd rather not touch this part now unless I'm sure that it works for all cases > (data: URI frames, about:srcdoc frames, etc.). We can do one of two things: (1) > go through the effort of testing of that as part of this change; or (2) fix the > bug that has been reported with just these changes, then open another ticket for > further work to simplify this code. > > What do you think? Well fine by me if you worry about improving this code separately, but please could you update the comment now at least? As it stands it's kind of misleading, since we do now clear the old style sheets.
https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... File lib/contentFiltering.js (left): https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... lib/contentFiltering.js:116: // Ideally we would compare the old and new style sheets and skip this code On 2018/12/19 13:07:15, kzar wrote: > On 2018/12/19 13:03:54, Manish Jethani wrote: > > On 2018/12/19 10:22:00, kzar wrote: > > > This comment implies we can simplify the logic here now that we know the > > > injectedStyleSheets for a frame will be cleared? > > > > I'd rather not touch this part now unless I'm sure that it works for all cases > > (data: URI frames, about:srcdoc frames, etc.). We can do one of two things: > (1) > > go through the effort of testing of that as part of this change; or (2) fix > the > > bug that has been reported with just these changes, then open another ticket > for > > further work to simplify this code. > > > > What do you think? > > Well fine by me if you worry about improving this code separately, but please > could you update the comment now at least? As it stands it's kind of misleading, > since we do now clear the old style sheets. It says "the old style sheet can be a leftover from a previous instance". This is exactly what I suspect may be the case for certain edge cases (data: URI frames, etc.), because we get different events for these. For these edge cases, the bug would not be fixed with this change (but at least it would be fixed for the simple case given in the bug report). So I think the comment reflects my concern, any suggestions on how we could update it?
https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... File lib/contentFiltering.js (left): https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... lib/contentFiltering.js:116: // Ideally we would compare the old and new style sheets and skip this code On 2018/12/19 13:27:10, Manish Jethani wrote: > On 2018/12/19 13:07:15, kzar wrote: > > On 2018/12/19 13:03:54, Manish Jethani wrote: > > > On 2018/12/19 10:22:00, kzar wrote: > > > > This comment implies we can simplify the logic here now that we know the > > > > injectedStyleSheets for a frame will be cleared? > > > > > > I'd rather not touch this part now unless I'm sure that it works for all > cases > > > (data: URI frames, about:srcdoc frames, etc.). We can do one of two things: > > (1) > > > go through the effort of testing of that as part of this change; or (2) fix > > the > > > bug that has been reported with just these changes, then open another ticket > > for > > > further work to simplify this code. > > > > > > What do you think? > > > > Well fine by me if you worry about improving this code separately, but please > > could you update the comment now at least? As it stands it's kind of > misleading, > > since we do now clear the old style sheets. > > It says "the old style sheet can be a leftover from a previous instance". This > is exactly what I suspect may be the case for certain edge cases (data: URI > frames, etc.), because we get different events for these. For these edge cases, > the bug would not be fixed with this change (but at least it would be fixed for > the simple case given in the bug report). > > So I think the comment reflects my concern, any suggestions on how we could > update it? For some context, all of this was originally done for element hiding emulation: https://issues.adblockplus.org/ticket/5864 But we don't use style sheets for element hiding emulation anymore. I think we can simplify this code, I just want to do it as a separate low-priority exercise (this bug on the other hand seems serious).
https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... File lib/contentFiltering.js (left): https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... lib/contentFiltering.js:116: // Ideally we would compare the old and new style sheets and skip this code On 2018/12/19 13:27:10, Manish Jethani wrote: > So I think the comment reflects my concern, any suggestions on how we could > update it? Sure, how about this? // Ideally we would compare the old and new style sheets and skip this code // if they're the same. But first, we need to ensure that there are no edge // cases which would cause the old style sheet to be a leftover from a // previous instance of the frame. See issue XXXX.
Patch Set 4: Update comment Filed this: https://issues.adblockplus.org/ticket/7180 https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... File lib/contentFiltering.js (left): https://codereview.adblockplus.org/29958567/diff/29964555/lib/contentFilterin... lib/contentFiltering.js:116: // Ideally we would compare the old and new style sheets and skip this code On 2018/12/19 13:35:42, kzar wrote: > On 2018/12/19 13:27:10, Manish Jethani wrote: > > So I think the comment reflects my concern, any suggestions on how we could > > update it? > > Sure, how about this? > > // Ideally we would compare the old and new style sheets and skip this code > // if they're the same. But first, we need to ensure that there are no edge > // cases which would cause the old style sheet to be a leftover from a > // previous instance of the frame. See issue XXXX. Done.
Nice, thanks. LGTM 
|
