Fix for ID3v2 Frame Read Failures (Lake Track) (#114)

* Make frame factory a class

* Fix issue of broken frame reading.

* Forgot to commit this one

Author: Benjamin Russell

src/id3v2/frames/frameFactory.ts

@@ -30,15 +30,17 @@ import {Guards, NumberUtils} from "../../utils";
  */
 export type FrameCreator = (data: ByteVector, offset: number, header: Id3v2FrameHeader, version: number) => Frame;
 
-let customFrameCreators: FrameCreator[] = [];
-
 /**
  * Performs the necessary operations to determine and create the correct child classes of
  * {@link Frame} for a given raw ID3v2 frame.
  * By default, this will only load frames contained in the library. To add additional frames to the
  * process, register a frame creator with {@link addFrameCreator}.
  */
-export default {
+export class Id3v2FrameFactory {
+
+
+    private static readonly CUSTOM_FRAME_CREATORS: FrameCreator[] = [];
+
     /**
      * Adds a custom frame creator to try before using standard frame creation methods.
      * Frame creators are used before standard methods so custom checking can be used and new
@@ -50,17 +52,17 @@ export default {
      *     * version: number ID3v2 version the raw frame data is stored in (should be byte)
      *     * returns Frame if method was able to match the frame, falsy otherwise
      */
-    addFrameCreator: (creator: FrameCreator): void => {
+    public static addFrameCreator(creator: FrameCreator): void {
         Guards.truthy(creator, "creator");
-        customFrameCreators.unshift(creator);
-    },
+        this.CUSTOM_FRAME_CREATORS.unshift(creator);
+    }
 
     /**
      * Removes all custom frame creators
      */
-    clearFrameCreators: (): void => {
-        customFrameCreators = [];
-    },
+    public static clearFrameCreators(): void {
+        this.CUSTOM_FRAME_CREATORS.length = 0;
+    }
 
     /**
      * Creates a {@link Frame} object by reading it from raw ID3v2 frame data.
@@ -78,13 +80,13 @@ export default {
      *     * offset: updated offset where the next frame starts
      */
     // @TODO: Split into fromFile and fromData
-    createFrame: (
+    public static createFrame(
         data: ByteVector,
         file: File,
         offset: number,
         version: number,
         alreadyUnsynced: boolean
-    ): {frame: Frame, offset: number} => {
+    ): {frame: Frame, offset: number} {
         Guards.uint(offset, "offset");
         Guards.byte(version, "version");
 
@@ -132,7 +134,7 @@ export default {
 
         try {
             // Try to find a custom creator
-            for (const creator of customFrameCreators) {
+            for (const creator of this.CUSTOM_FRAME_CREATORS) {
                 // @TODO: If we're reading from a file, data will only ever contain the header
                 const frame = creator(data, position, header, version);
                 if (frame) {
@@ -243,4 +245,4 @@ export default {
             };
         }
     }
-};
+}

src/id3v2/id3v2Tag.ts

@@ -1,6 +1,5 @@
 import AttachmentFrame from "./frames/attachmentFrame";
 import CommentsFrame from "./frames/commentsFrame";
-import FrameFactory from "./frames/frameFactory";
 import Id3v2ExtendedHeader from "./id3v2ExtendedHeader";
 import Id3v2TagFooter from "./id3v2TagFooter";
 import Id3v2Settings from "./id3v2Settings";
@@ -11,6 +10,7 @@ import {ByteVector, StringType} from "../byteVector";
 import {CorruptFileError, NotImplementedError, NotSupportedError} from "../errors";
 import {File, ReadStyle} from "../file";
 import {Frame, FrameClassType} from "./frames/frame";
+import {Id3v2FrameFactory} from "./frames/frameFactory";
 import {FrameIdentifier, FrameIdentifiers} from "./frameIdentifiers";
 import {Id3v2FrameFlags} from "./frames/frameHeader";
 import {Id3v2TagHeader, Id3v2TagHeaderFlags} from "./id3v2TagHeader";
@@ -1394,6 +1394,7 @@ export default class Id3v2Tag extends Tag {
         let frameDataEndPosition = (data ? data.length : this._header.tagSize) + frameDataPosition;
 
         // Check for the extended header
+        // @TODO: Replace with NumberUtils.hasFlags
         if ((this._header.flags & Id3v2TagHeaderFlags.ExtendedHeader) !== 0) {
             this._extendedHeader = Id3v2ExtendedHeader.fromData(data, this._header.majorVersion);
 
@@ -1405,28 +1406,41 @@ export default class Id3v2Tag extends Tag {
 
         // Parse the frames
         while (frameDataPosition < frameDataEndPosition) {
-            const frameRead = FrameFactory.createFrame(
-                data,
-                file,
-                frameDataPosition,
-                this._header.majorVersion,
-                fullTagUnsync
-            );
+            try {
+                const frameRead = Id3v2FrameFactory.createFrame(
+                    data,
+                    file,
+                    frameDataPosition,
+                    this._header.majorVersion,
+                    fullTagUnsync
+                );
+
+                // If the frame factory returned undefined, that means we've hit the end of frames
+                if (!frameRead) {
+                    break;
+                }
 
-            // If the frame factory returned undefined, that means we've hit the end of frames
-            if (!frameRead) {
-                break;
-            }
+                // We found a frame, deconstruct the read result
+                const frame = frameRead.frame;
+                frameDataPosition = frameRead.offset;
+
+                // Only add frames that contain data
+                if (!frame || frame.size === 0) {
+                    continue;
+                }
+                this.addFrame(frame);
+            } catch (e: unknown) {
+                // If we fail at any point while trying to read the frames of the tag, we will have
+                // lost our place in the file and will have to give up reading the tag.
+                // Ok, technically we could use some heuristics to try to recover (eg, Foobar can
+                // do this), but it is a lot of effort for minimal reward.
 
-            // We found a frame, deconstruct the read result
-            const frame = frameRead.frame;
-            frameDataPosition = frameRead.offset;
+                // NOTE: It's important to not let this error propagate to the file level, else
+                // the user will not be able to recover the file.
 
-            // Only add frames that contain data
-            if (!frame || frame.size === 0) {
-                continue;
+                // this.markCorrupt(e); @TODO: Add corruption reasons.
+                break;
             }
-            this.addFrame(frame);
         }
     }
 

src/index.ts

@@ -98,8 +98,8 @@ export {
     FrameClassType as Id3v2FrameClassType
 } from "./id3v2/frames/frame";
 export {
-    default as Id3v2FrameFactory,
-    FrameCreator as Id3v2FrameCreator
+    FrameCreator as Id3v2FrameCreator,
+    Id3v2FrameFactory
 } from "./id3v2/frames/frameFactory";
 export {Id3v2FrameFlags, Id3v2FrameHeader} from "./id3v2/frames/frameHeader";
 export {default as Id3v2MusicCdIdentifierFrame} from "./id3v2/frames/musicCdIdentifierFrame";