Skip to content

Fix export data#1532

Closed
lemnis wants to merge 5 commits into
EFForg:masterfrom
lemnis:fix-export-data
Closed

Fix export data#1532
lemnis wants to merge 5 commits into
EFForg:masterfrom
lemnis:fix-export-data

Conversation

@lemnis

@lemnis lemnis commented Jul 25, 2017

Copy link
Copy Markdown
Contributor

Should fix #1318

@ghostwords
ghostwords self-requested a review July 25, 2017 13:57

@ghostwords ghostwords left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that the fix does indeed fix the issue on Chrome, and that the fix makes exporting work better on Firefox.

Unfortunately, I can't approve this until we find a workaround that doesn't require asking for the "downloads" permission, which, I just confirmed, disables Privacy Badger upon upgrade until the user grants Privacy Badger the additional permission:

screenshot from 2017-07-26 12 22 11

@ghostwords

Copy link
Copy Markdown
Member

To test adding additional permissions in Chrome, see Testing permission warnings in Chrome Extensions developer documentation. You can package our extension by running make in the extension's root folder. Here is an example update manifest I used:

<?xml version='1.0' encoding='UTF-8'?>
<gupdate xmlns='http://www.google.com/update2/response' protocol='2.0'>
  <app appid='GET-THIS-ID-FROM-THE-CHROME-EXTENSIONS-PAGE-AFTER-DRAGGING-AND-DROPPING-THE-CRX'>
    <updatecheck codebase='http://localhost:9999/privacy_badger-2017.7.25.crx' version='2017.7.25' />
  </app>
</gupdate>

@lemnis

lemnis commented Jul 26, 2017

Copy link
Copy Markdown
Contributor Author

The previous method also supports blobs and fixes the bug within chrome, but will not work in Firefox (bugzilla bug report).

While I don't know the previous discussion around changing this specific code, I think adding this permission and as result, an automatic disabling of the extension will benefit the user in the long run.

@ghostwords

Copy link
Copy Markdown
Member

I don't think we should make users go through the disable-and-reenable process, which will result in losing a significant percentage of our userbase, unless we absolutely have to. This bug isn't high profile enough to warrant this step; there may yet be another workaround; PR #1462 should make the issue disappear for most people.

@ghostwords

Copy link
Copy Markdown
Member

The previous method also supports blobs and fixes the bug within chrome, but will not work in Firefox

I am not sure what you mean by this, could you elaborate please?

@lemnis

lemnis commented Jul 26, 2017

Copy link
Copy Markdown
Contributor Author

Look at the latest commit, easier than me explaining it

@ghostwords
ghostwords self-requested a review July 26, 2017 20:42
@lemnis

lemnis commented Jul 26, 2017

Copy link
Copy Markdown
Contributor Author

The code must switch to a blob, otherwise, the bug will keep creeping up. How chrome is coded, an url can only be a certain length, a blob will fix this.

@ghostwords ghostwords left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you say that the blob approach doesn't work in Firefox? I just tested the simplified code below on Firefox 54, and it seemed to work fine:

diff --git a/src/js/options.js b/src/js/options.js
index 2b82618..3dd8abc 100644
--- a/src/js/options.js
+++ b/src/js/options.js
@@ -183,24 +183,11 @@ function exportUserData() {
     var a = document.createElement('a');
     a.setAttribute('download', filename || '');
 
-    if(chrome.runtime.getBrowserInfo){
-      chrome.runtime.getBrowserInfo((info) => {
-        if(info.name == "Firefox"){
-          a.href = 'data:application/json;charset=utf-8,' + encodeURIComponent(mapJSON);
-          a.dispatchEvent(new MouseEvent('click'));
-        } else {
-          var blob = new Blob([mapJSON], {type: 'application/json'}); // pass a useful mime type here
-          a.href = URL.createObjectURL(blob);
-          a.dispatchEvent(new MouseEvent('click'));
-          URL.revokeObjectURL(blob);
-        }
-      });
-    } else {
-      var blob = new Blob([mapJSON], {type: 'application/json'}); // pass a useful mime type here
-      a.href = URL.createObjectURL(blob);
-      a.dispatchEvent(new MouseEvent('click'));
-      URL.revokeObjectURL(blob);
-    }
+    var blob = new Blob([mapJSON], {type: 'application/json'}); // pass a useful mime type here
+    a.href = URL.createObjectURL(blob);
+    a.dispatchEvent(new MouseEvent('click'));
+    URL.revokeObjectURL(blob);
+
   });
 }

@ghostwords

Copy link
Copy Markdown
Member

OK, I see, you linked to a Bugzilla issue above that itself is marked as a duplicate of https://bugzilla.mozilla.org/show_bug.cgi?id=1331176. It seems like downloading blobs doesn't work in Firefox 52, but might have been fixed some time since then.

@ghostwords ghostwords left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK cool, let's merge this. Thank you for the fix! I'll add a note to remove the conditional once Firefox 52 goes away (it's currently the ESR version).

@ghostwords

Copy link
Copy Markdown
Member

Merged in cde3fd8. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't export user data when data grows beyond a certain size

2 participants