Issue 7104 - Clear style sheets when frame structure is updated

lib/contentFiltering.js

Index: lib/contentFiltering.js
===================================================================
--- a/lib/contentFiltering.js
+++ b/lib/contentFiltering.js
@@ -100,20 +100,20 @@
 
 function updateFrameStyles(tabId, frameId, styleSheet, groupName = "standard",
                            appendOnly = false)
 {
   let frame = ext.getFrame(tabId, frameId);
   if (!frame)
     return false;
 
-  if (!frame.injectedStyleSheets)
-    frame.injectedStyleSheets = new Map();
+  if (!frame.state.injectedStyleSheets)
+    frame.state.injectedStyleSheets = new Map();
 
-  let oldStyleSheet = frame.injectedStyleSheets.get(groupName);
+  let oldStyleSheet = frame.state.injectedStyleSheets.get(groupName);
 
   if (appendOnly && oldStyleSheet)
     styleSheet = oldStyleSheet + styleSheet;
 
   // Ideally we would compare the old and new style sheets and skip this code

[kzar, 2018/12/19 10:22:00]

This comment implies we can simplify the logic here now that we know the
injectedStyleSheets for a frame will be cleared?

[Manish Jethani, 2018/12/19 13:03:54]

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?

[kzar, 2018/12/19 13:07:15]

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.

[Manish Jethani, 2018/12/19 13:27:10]

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?

[Manish Jethani, 2018/12/19 13:29:53]

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).

[kzar, 2018/12/19 13:35:42]

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.

[Manish Jethani, 2018/12/19 13:48:36]

Done.

   // 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.
@@ -129,17 +129,17 @@
   if (oldStyleSheet && oldStyleSheet != styleSheet)
     removeStyleSheet(tabId, frameId, oldStyleSheet);
 
   // The standard style sheet is ~660 KB per frame (as of Adblock Plus 3.3.2).
   // Keeping it in memory would only really be useful on Firefox, which allows
   // us to remove it via the tabs.removeCSS API. By choosing not to hold on to
   // it, we save potentially several megabytes per tab (#6967).
   if (groupName != "standard")
-    frame.injectedStyleSheets.set(groupName, styleSheet);
+    frame.state.injectedStyleSheets.set(groupName, styleSheet);
   return true;
 }
 
 function getExecutableCode(script)
 {
   let code = executableCode.get(script);
   if (code)
     return code;