Unsafe access to actorData.effects in prepare.js#432

Issue #244 new
Former user created an issue

Hello,

Using the latest version i'm running exception (per creature) when opening imported compendiums (for reproduction purposes, importing using Plutonium works) This doesn't prevent the feature from working however the very large number of exceptions to throw massively slow down the execution.

Adding a simple check around the loop is sufficient to prevent the issue if (actorData.effects != null) {...} however the better way would be proper initialization (and I don't know where to put it)

Full stack trace : TypeError: Failed to initialize data for ObsidianActor $id ($name): Cannot read property 'forEach' of undefined} at Object.conditions (VM368 prepare.js:432) at ObsidianActor.prepareDerivedData (actor.js:242) at ObsidianActor.prepareData (foundry.js:30700) at Function._applyActiveEffects_getFullRollData (PatcherActor.js:1) at PatcherActor.js:1 at Array.forEach (<anonymous>) at Function._applyActiveEffects_mutValues (PatcherActor.js:1) at Function._applyActiveEffects (PatcherActor.js:1) at ObsidianActor.<anonymous> (PatcherActor.js:1) at ObsidianActor._0x46111a (PatcherLibWrapper.js:1)

Comments (4)

  1. Scodde

    It’s possible, however i’m not certain it’s quite so simple.

    The imported creatures behaved fine “up to a point” (I haven’t identified the trigger, but I didn’t do anything outstanding, just DM-ing with the occasional restart) and in many cases a fully working Actor “stopped” working at some point. This has been the case both from Compendium and from instantiated Actor (#245 has proven to be a consequence of this as you presumed). From what little I’ve observed however, once broken it remains so => I’m really tempted to think that construction of this specific element isn’t handled properly in some cases.

    Once elements are in this state, even the result of “convert to obsidian”, while apparently successful (new compendium created, populated, etc) actually suffer from the same issue.

    I’m running the latest version (according to Foundry) : 4.4.10, with Foundry 0.7.9

    As far as I can tell this is very specific to actorData.effects (but then cascades to derivedElements, etc) and the following fixed all issues, including #245

    My first time doing Javascript (or looking into Obsidian structure), please forgive the improper implementation.

    Resetting prior to the .forEach is definitely the right way to do it, but I don’t know where the field is actually intended to be set.

    +   if (actorData.effects) {
          actorData.effects.forEach(effect => {
            const id = effect.flags?.core?.statusId;
            if (!id) {
              return;
            }
    
            if (id.startsWith('exhaust')) {
              const level = Number(id.substr(7));
              if (level > derived.conditions.exhaustion) {
                derived.conditions.exhaustion = level;
              }
    
              return;
            }
    
            derived.conditions[id] = !conditionImmunities.has(id);
          });
    +   }
    +   else{
    +     actorData.effects = []; 
    +  }
    

    This retro-actively fixed all my broken actors & compendiums, although it fails to explain how this element sometimes broke.

    Note that the else part, while ugly, was actually required for the fix to work, otherwise it just delays the exception to the next access.

  2. Kim Mantas repo owner

    I think it’s pretty unusual for actor.data.effects to ever not be an array. These embedded entity collections are handled by foundry itself and should always be initialised as an empty array (if no embedded entities exist). The only way I can even think that you could do that is if you forcefully deleted the entire property with actor.update({'-=effects': null}) but there’s no reason to ever do that except by mistake. That said, even the dnd5e system seems to expect effects to sometimes not exist (though I think that’s only because of either defensive programming or because it expects to have to migrate older actors) so perhaps obsidian should too.

    Given that I’ve never experienced this myself in my own testing and that yours is the only report (so far) of this nature, my money’s still on this being caused by some other module that’s messing with effects (and deleting them all), rather than obsidian.

  3. Scodde

    Probably, but within the scope of this repository I’d argue the real question isn’t “is it erroneously deleting effects and thus corrupting data ?” but rather “is this state of data [of null effects] valid as per Foundry core, and thus should be expected/handled by every module ?”

    I honestly don’t have the answer, but if it’s the latter, issue is valid regardless of root cause.

  4. Log in to comment