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

Store iconBlob as an array buffer #2909

Closed
wants to merge 2 commits into from

Conversation

Sxderp
Copy link
Contributor

@Sxderp Sxderp commented Mar 17, 2018

fixes #2908

There's a very minor bug with this PR that I can't seem to fix. On startup the scripts are force re-saved (updated eval content version) in order to save the buffer into the database. However, on this initial load the icons break and become Firefox placeholders. Reloading Greasemonkey resolves the issue. Further, the issue does not occur if you force re-save (update eval content version) again (for future updates).

Perhaps someone else can figure out the above bug. But since it occurs once I don't think it's that big of a deal.

@arantius arantius added this to the 4.4 milestone Mar 17, 2018
Ensures the following conditional doesn't throw if foundDetails is
null or undefined.
Copy link
Collaborator

@arantius arantius left a comment

Choose a reason for hiding this comment

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

How does this fix #2908?

@@ -92,6 +99,11 @@ async function loadUserScripts() {
};
}).then(loadDetails => {
let savePromises = loadDetails.map(details => {
if (details.iconBlob && ! (details.iconBlob instanceof Blob)) {
let info = details.iconBlob;
details.iconBlob = new Blob([info.buffer], {'type': info.type});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to do this both here and line 68-72? Does one or the other only handle updating things already saved the other way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arantius the 68-72 portion deals with the instance where we search the database for a script with an existing id in order to update it. Before we can update the just fetched script we need to change the array buffer into a blob.

The other is for when loading the scripts from initial start. Before we do anything with them we transform the array buffer into a blob.

Both instance ensure we don't do anything if the saved object contains a blob (meaning we're still in the progress of migrating).

The lines at 68-72 wouldn't be required if the script wasn't fetched from the database. But instead found from the already loaded scripts.

Either way, once the icons are stored as array buffers you need to convert them to blobs as soon as they're fetched from the database so you don't have inconsistencies or weird errors in other places.

@Sxderp
Copy link
Contributor Author

Sxderp commented Mar 23, 2018

How does this fix #2908 ?

There's something wrong with storing the icon as a blob. I don't know exactly what as blobs should be supported. The error was easily reproducible using a bare script with some random iconurl.

By switching to storing as an array buffer I was unable to reproduce the issue linked. The I did encounter the minor display issue as noted in comment 0.

@arantius
Copy link
Collaborator

Merged by hand.

@arantius arantius closed this Mar 28, 2018
@Sxderp Sxderp deleted the store-iconblob-as-buffer branch March 31, 2018 16:24
arantius added a commit to arantius/greasemonkey that referenced this pull request May 18, 2018
See greasemonkey#2909 where this was fixed only for `@icon`.  Now, store every blob in IndexedDB as an ArrayBuffer instead.  Parse it back into a blob in memory.
This is a work around which should be unnecessary but helps alleviate real bugs.

Fixes greasemonkey#2943
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.

Script save error toggling enabled state on some user scripts
2 participants