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

Breaks downloads #17

Closed
rugk opened this issue Oct 7, 2018 · 15 comments
Closed

Breaks downloads #17

rugk opened this issue Oct 7, 2018 · 15 comments

Comments

@rugk
Copy link

rugk commented Oct 7, 2018

I've been investigating a Firefox issue and it turned out your addon was causing it. If I have to guess, you somehow do not close/finish a request when it is cancelled or so…

So here are the full details, STR and so on: https://bugzilla.mozilla.org/show_bug.cgi?id=1480745

This is a very annoying minor bug that you'll get annoyed about each time you see it. 😄

@tasn
Copy link
Owner

tasn commented Oct 8, 2018

Thanks for reporting. I can't believe this is an issue with signed pages, it was annoying me too! :)

I'll try to figure it out, thanks for reporting!

@rugk
Copy link
Author

rugk commented Oct 8, 2018

Well… just disable the add-on and it works again, immediately. 😆

@tasn
Copy link
Owner

tasn commented Oct 8, 2018

💔

@rugk rugk changed the title Prevents loading icon at tab from stopping when download request is cancelled Breaks downloads Oct 8, 2019
@rugk
Copy link
Author

rugk commented Oct 8, 2019

This is really critical, as I just noticed…
In Firefox 69.0.1 it often breaks long-running (or big file?) downloads and I get this error:

/tmp/.../....part cannot be saved, because the source file cannot be read.

Originally reported at https://bugzilla.redhat.com/show_bug.cgi?id=1758895

@tasn
Copy link
Owner

tasn commented Oct 8, 2019

Oh, thanks for the update!
Having just reviewed the code following your report there's indeed a problem. It seems like we just try to process and validate any "main_frame", though downloads (and icons?) can also be ones.
While reviewing the code I also spotted a few other issues:

  1. ondata may be called more than once (e.g. partial data every time) - we need to address that.
  2. We need to make sure the response is a utf-8 one rather than binary before we decide to process it - need to verify how this should be done exactly. This should fix this issue.

Are you able to consistently reproduce the download issue? If so, are you able to test a potential workaround?

@tasn
Copy link
Owner

tasn commented Oct 8, 2019

Also, are you able to provide a link that fails? Or show the extension's output log? (Tools -> Web developer -> browser console)

@rugk
Copy link
Author

rugk commented Oct 8, 2019

Also maybe fix that TODO lol:

// FIXME: Only filter pages that we care about, the rest can skip this.

When the signature is not found, you can also add an if to return early instead of still decoding the text…

@rugk
Copy link
Author

rugk commented Oct 8, 2019

Maybe something like #23??

@tasn
Copy link
Owner

tasn commented Oct 9, 2019

From #23: Could you please try if this branch fixes it for you: https://github.com/tasn/webext-signed-pages/tree/early-abort

I added an early abort in case we shouldn't check a page.

@rugk
Copy link
Author

rugk commented Oct 9, 2019

Do you have a prebuild ZIP or so?

@tasn
Copy link
Owner

tasn commented Oct 9, 2019 via email

@rugk
Copy link
Author

rugk commented Oct 9, 2019

Yeah, I can use about:debugging. That's easy…

@tasn
Copy link
Owner

tasn commented Oct 9, 2019

Try this: signed_pages-0.4.1.zip

@rugk
Copy link
Author

rugk commented Oct 10, 2019

Hmm… could not reproduce the issue anymore right now… 🤔
But I guess it is fine… 😉

@tasn tasn closed this as completed in 743e0f6 Oct 10, 2019
@tasn
Copy link
Owner

tasn commented Oct 10, 2019

Released a new version.

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

No branches or pull requests

2 participants