Issue #2790 - Potential null-deref crash in EntityEditMaster after-load listener (unguarded e.detail.shadowRoot)

platform-web-ui/src/main/java/ua/com/fielden/platform/web/view/master/api/with_master/impl/AbstractMasterWithMaster.java

@@ -64,7 +64,8 @@ public AbstractMasterWithMaster(final Class<T> entityType, final Class<? extends
                         "   if (_currBindingEntity !== null) {\n" +
                         "       return " + getAttributes(embededMasterType, "_currBindingEntity", shouldRefreshParentCentreAfterSave) +
                         "   };\n" +
-                        "}).bind(this);\n")
+                        "}).bind(this);\n" +
+                        getAdditionalReadyCallbackCode())
                 .replace("//@attached-callback",
                           "this.canLeave = async function (leaveReason) {"
                         + "    const embeddedMaster = this.$.loader.loadedElement;\n"
@@ -93,6 +94,17 @@ protected List<String> getAdditionalImports() {
         return new ArrayList<>();
     };
 
+    /**
+     * Returns additional JS code to be appended to the `ready` callback of this master.
+     * Both `this` and `self` refer to the master element at that point.
+     *
+     * This method is invoked from the constructor.
+     * Overriding implementations must return a constant, and must not depend on subclass state.
+     */
+    protected String getAdditionalReadyCallbackCode() {
+        return "";
+    }
+
     /**
      * Returns the implementation for the after load listener of embedded master.
      *

platform-web-ui/src/main/java/ua/com/fielden/platform/web/view/master/api/with_master/impl/EntityEditMaster.java

@@ -8,7 +8,7 @@
 /**
  * Custom entity master for all edit actions.
  * <p>
- * Firstly, it ensures that it will not be closed on SAVE / CANCEL buttons of embedded master (see closeAfterExecution property setting).
+ * Firstly, it ensures that it will not be closed on SAVE / CANCEL buttons of embedded master (see {@link #shouldCloseAfterSave()}).
  * Secondly, it adds support for navigation between heterogenic entities. Use {@link EntityNavigationPreAction} to enable navigation.
  *
  * @author TG Team
@@ -21,36 +21,20 @@ public class EntityEditMaster extends EntityManipulationMaster<EntityEditAction>
     public EntityEditMaster(final Class<EntityEditAction> entityType, final boolean shouldRefreshParentCentreAfterSave) {
         super(entityType, shouldRefreshParentCentreAfterSave);
         final String masterTemplate = super.render().render().toString().replace("//@master-is-ready-custom-code",
-                "             //Provide custom after load listener\n"
-              + "             self._seqEditAfterLoadListener = function (e) {\n"
-              + "                 this._assignPostSavedHandlersForEmbeddedMaster(e);\n"
-              + "                 const saveButton = e.detail.shadowRoot.querySelector(\"tg-action[role='save']\");\n"
-              + "                 if (saveButton) {\n"
-              + "                     saveButton.closeAfterExecution = false;\n"
-              + "                 }\n"
-              + "             }.bind(self);\n"
-              + "             self._handleBindingEntityChanged = function (e) {\n"
-              + "                 if (e.detail.value && e.detail.value.entityType) {\n"
-              + "                     if (this._prevCurrBindingEntity && e.detail.value.entityType !== this._prevCurrBindingEntity.entityType) {\n"
-              + "                         this.fire('tg-master-type-before-change',{\n"
-              + "                             prevType: this._prevCurrBindingEntity.entityType,\n"
-              + "                             currType: e.detail.value.entityType\n"
-              + "                         });\n"
-              + "                     }\n"
-              + "                     this._prevCurrBindingEntity = e.detail.value;\n"
-              + "                 }\n"
-              + "             }.bind(self);\n"
+                "             //Report a failure to retrieve the entity being navigated to\n"
               + "             self._handleBindingEntityRetrievedError = function (e) {\n"
               + "                 this.fire('tg-master-navigation-error');\n"
               + "             }.bind(self);\n"
-              + "             self.addEventListener('_curr-binding-entity-changed', self._handleBindingEntityChanged);\n"
               + "             self.$.loader.addEventListener('binding-entity-retrieved-error', self._handleBindingEntityRetrievedError);\n");
       this.renderable = () -> new InnerTextElement(masterTemplate);
     }
 
+    /**
+     * Closing of the enclosing dialog is governed by this master, and not by the SAVE action of the embedded master.
+     */
     @Override
-    protected String getAfterLoadListener() {
-        return "this._seqEditAfterLoadListener";
+    protected boolean shouldCloseAfterSave() {
+        return false;
     }
 
     @Override

platform-web-ui/src/main/java/ua/com/fielden/platform/web/view/master/api/with_master/impl/EntityManipulationMaster.java

@@ -9,6 +9,44 @@ protected EntityManipulationMaster(final Class<T> entityType, final boolean shou
         super(entityType, null, shouldRefreshParentCentreAfterSave);
     }
 
+    /**
+     * Indicates whether the SAVE action of the embedded master may close the enclosing dialog.
+     * Masters that govern closing themselves should override this method to return `false`.
+     *
+     * The returned value is passed to the embedded master as property `shouldCloseAfterSave` (see `tg-entity-master-behavior`).
+     * `tg-element-loader` assigns that property before inserting the embedded master into the DOM, that is, before the embedded master gets connected.
+     * Therefore, the value is guaranteed to be in place by the time the embedded master's `ready` callback runs, which is where it takes effect.
+     *
+     * This method is invoked from a superclass constructor by way of `getAttributes`.
+     * Overriding implementations must return a constant, and must not depend on subclass state.
+     */
+    protected boolean shouldCloseAfterSave() {
+        return true;
+    }
+
+    /**
+     * Announces an imminent change of the embedded master's type by firing `tg-master-type-before-change`.
+     * The enclosing dialog reacts to this event (see `tg-custom-action-dialog._handleMasterBeforeChange`) to update its title and to start the resize animation before the new master is rendered.
+     *
+     * This applies to every manipulation master, as `entityType` is a property of {@link AbstractEntityManipulationAction}.
+     * The event is fired only when the type actually changes, so masters bound to a single entity type never fire it.
+     */
+    @Override
+    protected String getAdditionalReadyCallbackCode() {
+        return "this._handleBindingEntityChanged = function (e) {\n"
+             + "    if (e.detail.value && e.detail.value.entityType) {\n"
+             + "        if (this._prevCurrBindingEntity && e.detail.value.entityType !== this._prevCurrBindingEntity.entityType) {\n"
+             + "            this.fire('tg-master-type-before-change', {\n"
+             + "                prevType: this._prevCurrBindingEntity.entityType,\n"
+             + "                currType: e.detail.value.entityType\n"
+             + "            });\n"
+             + "        }\n"
+             + "        this._prevCurrBindingEntity = e.detail.value;\n"
+             + "    }\n"
+             + "}.bind(this);\n"
+             + "this.addEventListener('_curr-binding-entity-changed', this._handleBindingEntityChanged);\n";
+    }
+
     @Override
     protected String getAttributes(final Class<? extends AbstractEntity<?>> entityType, final String bindingEntityName, final boolean shouldRefreshParentCentreAfterSave) {
         return "{" +
@@ -17,6 +55,7 @@ protected String getAttributes(final Class<? extends AbstractEntity<?>> entityTy
                 "   excludeInsertionPoints: this.excludeInsertionPoints, " +
                 "   entityId: " + bindingEntityName + ".entityId, " +
                 "   entityType: " + bindingEntityName + ".entityType, " +
+                "   shouldCloseAfterSave: " + shouldCloseAfterSave() + ", " +
                 "   shouldRefreshParentCentreAfterSave: " + shouldRefreshParentCentreAfterSave +
                 "};";
     }

platform-web-ui/src/main/web/ua/com/fielden/platform/web/master/actions/tg-action.js

@@ -422,6 +422,11 @@ Polymer({
             const closeAfterExecution = this._options[itemIdx].closeAfterExecution;
             const subRole = this._options[itemIdx].subRole;
             this.async(function () {
+                // This action could have become disabled during the delay above -- see '_asyncRun' for the rationale.
+                // No exemption for programmatic invocations is needed here, as an option can only be activated by the user.
+                if (this.hasAttribute('action-disabled')) {
+                    return;
+                }
                 this.run(closeAfterExecution, subRole);
             }, 100);
         }
@@ -600,6 +605,14 @@ Polymer({
         // it is critical to execute the actual logic that is intended for an on-tap action in async
         // with a relatively long delay to make sure that all required changes
         this.async(function () {
+            // A user-initiated action gets admitted only while enabled, but it could have become disabled during the delay above.
+            // For example, a retrieval or a saving, initiated meanwhile, disables the master and, with it, all of its actions.
+            // Running such an action would issue a request concurrent with the one already in progress.
+            // Programmatic invocations, which pass neither 'e' nor 'shortcut', are exempt.
+            // This is because developer-driven code may legitimately run a disabled action, such as saving an unmodified entity (see '_createSavingPromise' in 'tg-entity-master-behavior').
+            if ((e || shortcut) && this.hasAttribute('action-disabled')) {
+                return;
+            }
             let subRole = '';
             let closeAfterExecution = this.closeAfterExecution;
             if (shortcut && this._options) {

platform-web-ui/src/main/web/ua/com/fielden/platform/web/master/tg-entity-master-behavior.js

@@ -499,6 +499,18 @@ const TgEntityMasterBehaviorImpl = {
             value: true
         },
 
+        /**
+         * Indicates whether the SAVE action of this master may close the enclosing dialog.
+         *
+         * A master that embeds this one and governs closing itself (e.g. the master for 'EntityEditAction') assigns 'false'.
+         * Such an assignment is made by 'tg-element-loader', which applies the loaded element's attributes before inserting
+         * that element into the DOM -- that is, before this master gets connected and, hence, before 'ready' runs.
+         */
+        shouldCloseAfterSave: {
+            type: Boolean,
+            value: true
+        },
+
         /**
          * The map of saved continuation functional entities by their property name identifiers.
          *
@@ -1017,9 +1029,9 @@ const TgEntityMasterBehaviorImpl = {
             }
         });
 
-        // Don't close this master on save action if it is a part of compound master.
+        // Don't close this master on save action if it is a part of compound master, or if an embedding master governs closing itself.
         const menuSectionParent = getParentAnd(self, element => element.matches('tg-master-menu-item-section'));
-        if (menuSectionParent) {
+        if (menuSectionParent || !self.shouldCloseAfterSave) {
             const saveButton = self.$._saveAction;
             if (saveButton) {
                 saveButton.closeAfterExecution = false;