Skip to content
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

Iron out bug fixes before new release #407

Closed
3 tasks done
OliverWang13 opened this issue Feb 27, 2023 · 9 comments
Closed
3 tasks done

Iron out bug fixes before new release #407

OliverWang13 opened this issue Feb 27, 2023 · 9 comments
Assignees
Labels
bug Something isn't working core functionality Core functionality that is crucial for purpose of the software

Comments

@OliverWang13
Copy link
Collaborator

OliverWang13 commented Feb 27, 2023

Like it says in #382, we are now in the process of testing the extension and finding issues. Here are a few that I have noticed. @sophieeng, if you could run some more tests and add if you find anything, that would be great.

  • The popup toggle does not work to stop sending the DOM signal on Chrome. This could perhaps be a timing issue.
  • The popup toggle for Firefox does not stop sending the headers.
  • In a few small cases I tested, the popup slider kept turning itself on.

For now, this is all I see, but I expect some more to pop up.

@OliverWang13 OliverWang13 added bug Something isn't working core functionality Core functionality that is crucial for purpose of the software labels Feb 27, 2023
OliverWang13 added a commit that referenced this issue Feb 27, 2023
@OliverWang13
Copy link
Collaborator Author

In this issue I also created another change that could be making some problems, @sophieeng. The changes are the two functions createCS and deleteCS, and these are meant to modify the content script matches without rebuilding them (which is what we currently do). I will try to experiment with this a bit more and get it working.

@OliverWang13
Copy link
Collaborator Author

I just pushed some changes that I will explain here.

Previously, our Chrome GPC DOM signal worked by running two content scripts, one that added it and one that removed it. The script that added it had all sites matching, and to make a site remove the signal we would add that domain to the second content script and it would immediately strip that one. This method seems to have gotten less effective at consistently performing.

Largely, this process was done in an inefficient manner where to update the second script we would iterate through our database looking for sites that were turned off and then setting the second script's matches to what we found. I just wrote up two more functions createCS and deleteCS that instead do this specifically for a domain, avoiding iterating through the whole domainlist.

Another larger change is that I completely removed the second content script and instead am now utilizing the excludeMatches aspect of the chrome.scripting.updateContentScripts function.

So far, the toggling on and off in the popup seems to be working for Chrome perfectly. Toggling on the domainlist seems to be working as well. However, the on/off popup button is not functioning as desired, so I will look into that further. It is definitely possible that in these changes one or two things fell through the cracks, so intensive testing would be helpful. In addition, our architecture readme will need to be updated to represent these changes, which I will open an issue for.

@SebastianZimmeck
Copy link
Member

Thanks for exploring this, @OliverWang13!

Previously, our Chrome GPC DOM signal worked by running two content scripts, one that added it and one that removed it.

After your changes, would we still follow the same strategy of adding and removing or just adding (only the sites for which GPC is enabled)?

To add also here, the reason was that sometimes our extension was not fast enough to set GPC on some sites.

OliverWang13 added a commit that referenced this issue Mar 2, 2023
@OliverWang13
Copy link
Collaborator Author

No, this will change the strategy so now we only add the GPC signal to the sites that are NOT specified in excludeMatches. So far, through some small performance tests on the reference server, it is performing quite well on Chrome.

@SebastianZimmeck
Copy link
Member

As @OliverWang13 clarified today, we are no longer using the two pronged strategy --- (1) adding GPC to all sites and (2) removing it from sites someone wants to remain opted in. This two-pronged strategy was initially helpful as we were not able to selectively attach GPC fast enough on some sites. We decided that it was better to having a user opted out on a site too many instead of having the user opted in on a site too many. The bottom line is that now simply adding GPC works. So, that is what we do.

@sophieeng
Copy link
Collaborator

More detail on the last task to fix:

  • Chrome headers and DOM signal is working as expected using the popup functionality.
  • The disable function on the Firefox popup is working as expected.

Here is what I notice on the Firefox extension using the toggle for the Do Not Sell signal:

  1. When you first load the unpacked extension to FF and simply navigate to the server, the header is not present but the DOM signal is. After lots of refreshing, the page stays like this. We expect to see both the header and DOM signal present.
  2. When you toggle off the Do Not Sell signal through the popup, the first time the server will show as expected with both the header and DOM signal turned off. But if you refresh the page, the header will turn back on and stay turned on. The DOM signal will not change, and still be turned off.
  3. When you toggle the signal on again through the extension, the server will show both the header and DOM signal present, which is expected. But if you start to refresh the server, the header will turn off and stay turned off. The DOM signal will not change, and still be turned on.
@SebastianZimmeck
Copy link
Member

Thanks, @sophieeng. That is strange, especially, as we have not switched to mv3 for Firefox yet. Can you look into other extensions, especially, DDG Privacy Essentials? Does it work for them? If so, what are they doing differently than we do?

@OliverWang13
Copy link
Collaborator Author

I believe I was able to resolve this Firefox header issue in the previous commit. There are no outstanding issues that I know of in the extension, and @sophieeng and I will run a few more final checks before finalizing the release. It is possible that an issue or two can pop up but currently our main extension functionality (headers, DOM) seems to be working properly.

OliverWang13 added a commit that referenced this issue Mar 23, 2023
@OliverWang13
Copy link
Collaborator Author

@sophieeng has tested the extension and has not found any new errors, so we will be closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core functionality Core functionality that is crucial for purpose of the software
3 participants