Fix (unreported) design flow in how workspace's relation data are read from .blend file.

Relying on pointer addresses across different data-blocks is extremely
not recommended (and should be strictly forbidden ideally), in
particular in direct_link step of blend file reading.
- It assumes a specific order in reading of data, which is not ensured
  in future, and is in any case a very bad, non explicit, hidden
  dependency on behaviors of other parts of the codebase.
- It is intrinsically unsafe (as in, it makes writing bad code and making
  mistakes easy, see e.g. fix in rB84b3f6e049b35f9).
- It makes advanced handling of data-blocks harder (thinking about
  partial undo code e.g., even though in this specific case it was not
  an issue as we do not re-read neither windowmanagers nor worspaces
  during undo).

New code uses windows' `winid` instead as 'anchor' to find again proper
workspace hook in windows at read time.

As a bonus, it will also cleanup the list of relations from any invalid
ones (afaict it was never done previously).

Differential Revision: https://developer.blender.org/D9073

4 files changed, 56 insertions, 13 deletions

diff --git a/source/blender/blenloader/intern/readfile.c b/source/blender/blenloader/intern/readfile.c
index 1072cd3686e..aa2f103c693 100644
--- a/source/blender/blenloader/intern/readfile.c
+++ b/source/blender/blenloader/intern/readfile.c
@@ -1794,7 +1794,7 @@ static void *newdataadr_no_us(FileData *fd, const void *adr)
 }
 
 /* direct datablocks with global linking */
-static void *newglobadr(FileData *fd, const void *adr)
+void *blo_read_get_new_globaldata_address(FileData *fd, const void *adr)
 {
   return oldnewmap_lookup_and_inc(fd->globmap, adr, true);
 }
@@ -2553,6 +2553,21 @@ static void lib_link_workspaces(BlendLibReader *reader, WorkSpace *workspace)
 {
   ID *id = (ID *)workspace;
 
+  /* Restore proper 'parent' pointers to relevant data, and clean up unused/invalid entries. */
+  LISTBASE_FOREACH_MUTABLE (WorkSpaceDataRelation *, relation, &workspace->hook_layout_relations) {
+    relation->parent = NULL;
+    LISTBASE_FOREACH (wmWindowManager *, wm, &reader->main->wm) {
+      LISTBASE_FOREACH (wmWindow *, win, &wm->windows) {
+        if (win->winid == relation->parentid) {
+          relation->parent = win->workspace_hook;
+        }
+      }
+    }
+    if (relation->parent == NULL) {
+      BLI_freelinkN(&workspace->hook_layout_relations, relation);
+    }
+  }
+
   LISTBASE_FOREACH_MUTABLE (WorkSpaceLayout *, layout, &workspace->layouts) {
     BLO_read_id_address(reader, id->lib, &layout->screen);
 
@@ -2581,12 +2596,8 @@ static void direct_link_workspace(BlendDataReader *reader, WorkSpace *workspace)
   BLO_read_list(reader, &workspace->tools);
 
   LISTBASE_FOREACH (WorkSpaceDataRelation *, relation, &workspace->hook_layout_relations) {
-    /* data from window - need to access through global oldnew-map */
-    /* XXX This is absolutely not acceptable. There is no acceptable reasons to mess with other
-     * ID's data in read code, and certainly never, ever in `direct_link_` functions.
-     * Kept for now because it seems to work, but it should be refactored. Probably store and use
-     * window's `winid`, just like it was already done for screens? */
-    relation->parent = newglobadr(reader->fd, relation->parent);
+    /* parent pointer does not belong to workspace data and is therefore restored in lib_link step
+     * of window manager.*/
     BLO_read_data_address(reader, &relation->value);
   }
 
@@ -5472,8 +5483,9 @@ static void direct_link_windowmanager(BlendDataReader *reader, wmWindowManager *
     WorkSpaceInstanceHook *hook = win->workspace_hook;
     BLO_read_data_address(reader, &win->workspace_hook);
 
-    /* we need to restore a pointer to this later when reading workspaces,
-     * so store in global oldnew-map. */
+    /* We need to restore a pointer to this later when reading workspaces,
+     * so store in global oldnew-map.
+     * Note that this is only needed for versionning of older .blend files now.. */
     oldnewmap_insert(reader->fd->globmap, hook, win->workspace_hook, 0);
 
     direct_link_area_map(reader, &win->global_areas);
@@ -6847,7 +6859,7 @@ static BHead *read_global(BlendFileData *bfd, FileData *fd, BHead *bhead)
 /* note, this has to be kept for reading older files... */
 static void link_global(FileData *fd, BlendFileData *bfd)
 {
-  bfd->cur_view_layer = newglobadr(fd, bfd->cur_view_layer);
+  bfd->cur_view_layer = blo_read_get_new_globaldata_address(fd, bfd->cur_view_layer);
   bfd->curscreen = newlibadr(fd, NULL, bfd->curscreen);
   bfd->curscene = newlibadr(fd, NULL, bfd->curscene);
   // this happens in files older than 2.35
diff --git a/source/blender/blenloader/intern/readfile.h b/source/blender/blenloader/intern/readfile.h
index 2ddf96a2d47..d3372a646a9 100644
--- a/source/blender/blenloader/intern/readfile.h
+++ b/source/blender/blenloader/intern/readfile.h
@@ -203,3 +203,7 @@ void do_versions_after_linking_270(struct Main *bmain);
 void do_versions_after_linking_280(struct Main *bmain, struct ReportList *reports);
 void do_versions_after_linking_290(struct Main *bmain, struct ReportList *reports);
 void do_versions_after_linking_cycles(struct Main *bmain);
+
+/* This is rather unfortunate to have to expose this here, but better use that nasty hack in
+ * do_version than readfile itself. */
+void *blo_read_get_new_globaldata_address(struct FileData *fd, const void *adr);
diff --git a/source/blender/blenloader/intern/versioning_280.c b/source/blender/blenloader/intern/versioning_280.c
index 14a4643f760..b7c48234590 100644
--- a/source/blender/blenloader/intern/versioning_280.c
+++ b/source/blender/blenloader/intern/versioning_280.c
@@ -241,7 +241,7 @@ static void do_version_workspaces_after_lib_link(Main *bmain)
       if (screen->temp) {
         /* We do not generate a new workspace for those screens...
          * still need to set some data in win. */
-        win->workspace_hook = BKE_workspace_instance_hook_create(bmain);
+        win->workspace_hook = BKE_workspace_instance_hook_create(bmain, win->winid);
         win->scene = screen->scene;
         /* Deprecated from now on! */
         win->screen = NULL;
@@ -254,10 +254,10 @@ static void do_version_workspaces_after_lib_link(Main *bmain)
       WorkSpaceLayout *layout = BKE_workspace_layout_find(workspace, win->screen);
       BLI_assert(layout != NULL);
 
-      win->workspace_hook = BKE_workspace_instance_hook_create(bmain);
+      win->workspace_hook = BKE_workspace_instance_hook_create(bmain, win->winid);
 
       BKE_workspace_active_set(win->workspace_hook, workspace);
-      BKE_workspace_active_layout_set(win->workspace_hook, workspace, layout);
+      BKE_workspace_active_layout_set(win->workspace_hook, win->winid, workspace, layout);
 
       /* Move scene and view layer to window. */
       Scene *scene = screen->scene;
diff --git a/source/blender/blenloader/intern/versioning_290.c b/source/blender/blenloader/intern/versioning_290.c
index 400403a4636..48ae5963da8 100644
--- a/source/blender/blenloader/intern/versioning_290.c
+++ b/source/blender/blenloader/intern/versioning_290.c
@@ -41,6 +41,7 @@
 #include "DNA_rigidbody_types.h"
 #include "DNA_screen_types.h"
 #include "DNA_shader_fx_types.h"
+#include "DNA_workspace_types.h"
 
 #include "BKE_collection.h"
 #include "BKE_colortools.h"
@@ -779,5 +780,31 @@ void blo_do_versions_290(FileData *fd, Library *UNUSED(lib), Main *bmain)
    */
   {
     /* Keep this block, even when empty. */
+    if (!DNA_struct_elem_find(fd->filesdna, "WorkSpaceDataRelation", "int", "parentid")) {
+      LISTBASE_FOREACH (WorkSpace *, workspace, &bmain->workspaces) {
+        LISTBASE_FOREACH_MUTABLE (
+            WorkSpaceDataRelation *, relation, &workspace->hook_layout_relations) {
+          relation->parent = blo_read_get_new_globaldata_address(fd, relation->parent);
+          BLI_assert(relation->parentid == 0);
+          if (relation->parent != NULL) {
+            LISTBASE_FOREACH (wmWindowManager *, wm, &bmain->wm) {
+              wmWindow *win = BLI_findptr(
+                  &wm->windows, relation->parent, offsetof(wmWindow, workspace_hook));
+              if (win != NULL) {
+                relation->parentid = win->winid;
+                break;
+              }
+            }
+            if (relation->parentid == 0) {
+              BLI_assert(
+                  !"Found a valid parent for workspace data relation, but no valid parent id.");
+            }
+          }
+          if (relation->parentid == 0) {
+            BLI_freelinkN(&workspace->hook_layout_relations, relation);
+          }
+        }
+      }
+    }
   }
 }