aadsm / jsmediatags

Media Tags Reader (ID3, MP4, FLAC)
Other
748 stars 128 forks source link

tag reading fails, breaking promise chain #37

Closed shukriadams closed 7 years ago

shukriadams commented 7 years ago

I've encountered a rare error, when reading a large number (25k+) of unsanitized mp3 files, a few of those files throw an unrecoverable error which breaks my program.

Details: MediaFileReader.js, line 67, throws an invalid array length exception on some mp3 files. The error is caused by variable "length" having a value of -2, which Array cannot be instantiated on.

Tracing the error further up the next point of interest is in ID3v2TagReader.js, line 160, framesize is 2, which is too low. Why it's this low I don't know.

Bigger picture : From what I can tell you're missing an error check in MediaTagReader.js, line 42 :

var tags = self._parseData(self._mediaFileReader, self._tags);  

This line tries to load tags after a successful load of the file, but this is too optimistic, given that tag reading can still fail on rare occasions. I "fixed" it with

        try
        {
          var tags = self._parseData(self._mediaFileReader, self._tags);  
          callbacks.onSuccess(tags);
        }catch(ex){
          callbacks.onError({type : 'foo', info : 'bar' });  
        }

Which continues the promise chain in the event of an error happening inside onSuccess. I'm not super clued up on promise theory, but I'm guessing this is an acceptable fix? I'd be happy to write and pull request it.

aadsm commented 7 years ago

Looks like a bug, are you able to figure out which mp3 causes that?

shukriadams commented 7 years ago

I can't see anything obviously wrong with the mp3s' that cause this - their tags work in Winamp for example. I can investigate a bit further, though I still like the idea that a tag reader can gracefully handle a corrupt file..

aadsm commented 7 years ago

Yeah, I don't think the mp3 is corrupt, I believe there's a bug in the library. If I could get a hold of the mp3 the library can't read I should be able to fix it.

On Monday, November 7, 2016, Shukri Adams notifications@github.com wrote:

I can't see anything obviously wrong with the mp3s' that cause this - their tags work in Winamp for example. I can investigate a bit further, though I still like the idea that a tag reader can gracefully handle a corrupt file..

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/aadsm/jsmediatags/issues/37#issuecomment-258805902, or mute the thread https://github.com/notifications/unsubscribe-auth/AAExKsFQMynFYxG87PCYk7ANZboTpkB5ks5q7wQEgaJpZM4KmImN .

Best regards, António Afonso

shukriadams commented 7 years ago

@aadsm Sorry about the delay replying. Can you please contact me at shukri.adams at gmail com. Then I can send you 1-2 files which are failing. Thanks.