Issue 6183 - Avoid attempting to close the composer popup twice

composer.postload.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 423 matching lines @@
   document.removeEventListener("mouseout", mouseOut, true);
   document.removeEventListener("click", elementPicked, true);
   document.removeEventListener("contextmenu", elementPicked, true);
   document.removeEventListener("keydown", keyDown, true);
 }
 
 
 /* Core logic */
 
 // We're done with the block element feature for now, tidy everything up.
-function deactivateBlockElement()
+function deactivateBlockElement(popupAlreadyClosed)
 {
   if (currentlyPickingElement)
     stopPickingElement();
 
-  if (blockelementPopupId != null)
+  if (blockelementPopupId != null && !popupAlreadyClosed)
   {
     browser.runtime.sendMessage({
       type: "forward",
       targetPageId: blockelementPopupId,
       payload:
       {
         type: "composer.dialog.close"
       }
     });
-
-    blockelementPopupId = null;
   }
 
+  blockelementPopupId = null;
   lastRightClickEvent = null;
 
   if (currentElement)
   {
     unhighlightElement(currentElement);
     currentElement = null;
   }
   unhighlightElements();
 
   let overlays = document.getElementsByClassName("__adblockplus__overlay");
@@ skipping 66 matching lines @@
           // Hide the selected element itself if an added blocking
           // filter is causing it to collapse. Note that this
           // behavior is incomplete, but the best we can do here,
           // e.g. if an added blocking filter matches other elements,
           // the effect won't be visible until the page is is reloaded.
           checkCollapse(currentElement.prisoner || currentElement);
 
           // Apply added element hiding filters.
           elemhide.apply();
         }
-        deactivateBlockElement();
+        deactivateBlockElement(!!message.popupAlreadyClosed);
         break;
       case "composer.content.clearPreviousRightClickEvent":
         if (!lastRightClickEventIsMostRecent)
           lastRightClickEvent = null;
         lastRightClickEventIsMostRecent = false;
         break;
       case "composer.content.dialogOpened":
         if (window == window.top)
           blockelementPopupId = message.popupId;
         break;
       case "composer.content.dialogClosed":
         // The onRemoved hook for the popup can create a race condition, so we
         // to be careful here. (This is not perfect, but best we can do.)
         if (window == window.top && blockelementPopupId == message.popupId)
         {
           browser.runtime.sendMessage({
             type: "forward",
             payload:
             {
-              type: "composer.content.finished"
+              type: "composer.content.finished",
+              popupAlreadyClosed: true

[Manish Jethani, 2018/02/20 11:17:30]

We could just call this "dialogClosed" (instead of "popupAlreadyClosed") to use
consistent terminology, i.e. composer.dialog.close,
composer.content.dialogClosed, etc.

Otherwise LGTM

[kzar, 2018/02/20 11:56:44]

FWIW I prefer popupAlreadyClosed so I went with that, but I can see the argument
either way.

[Manish Jethani, 2018/02/20 12:33:25]

Sounds good, thanks.

             }
           });
         }
         break;
     }
   });
 
   if (window == window.top)
     browser.runtime.sendMessage({type: "composer.ready"});
 
   return true;
 }
 
 if (document instanceof HTMLDocument)
 {
   // There's a bug in Firefox that causes document_end content scripts to run
   // before document_start content scripts on extension startup. In this case
   // the ext object is undefined, we fail to initialize, and initializeComposer
   // returns false. As a workaround, try again after a timeout.
   // https://bugzilla.mozilla.org/show_bug.cgi?id=1395287
   if (!initializeComposer())
     setTimeout(initializeComposer, 2000);
 }