1062014 - Add node existence check to CustomizeMode's onDragEnd

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

I've run into an issue where (because of the workaround I've had to implement due to bug 1058990), if a widget node is destroyed in mid-air after dragging it out of any area in customize mode, it can cause an exception in CustomizeMode's code here: http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#1829

> 1828     let draggedWrapper = document.getElementById("wrapper-" + draggedItemId);
> 1829     draggedWrapper.hidden = false;
> 1830     draggedWrapper.removeAttribute("mousedown");

The exception seems harmless, as everything seems to remain working correctly afterwards. But the solution is simple: add a small check to make sure draggedWrapper exists before proceeding with the next couple of declarations.

I realize this isn't a situation that the user would easily run into (or at all without my add-on causing it?), but it's not something I can fix within the add-on without doing some serious spelunking into the CustomizeMode and CustomizableUI code, when the fix from your side would be so incredibly simple. :) Unless of course if I missed something in the calls after that bit of code, or unless you believe this fix could disguise the cause of some potential breakage in the future.

Comment 1

[Luís Miguel [:quicksaver] (not reading bugmail)]

Another way to look at this is: destroyWidget() doesn't remove node if it's currently being dragged, as the drag handler keeps a valid reference to it and still appends it when it finishes.

Comment 2

[Ruth]

Hi,
Can I work on this bug?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Yes, you can! Do you have any questions on how to proceed, Ruth?

Comment 4

[Ruth]

Great! 
I understand that the solution is to add a simple check before the lines 1829 -1830. am I right?
However I couldn't reproduce the problem. How can one widget be destroyed in mid-air? It always returns to the initial position without any exception.

Nevertheless I already changed the code and built it successfuly. After your confirmation, I’ll attach the patch. Thanks!

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ruth from comment #4)
> Great! 
> I understand that the solution is to add a simple check before the lines
> 1829 -1830. am I right?
> However I couldn't reproduce the problem. How can one widget be destroyed in
> mid-air? It always returns to the initial position without any exception.

If the widget has a dragend event handler that removes it, the widget will not be available during the drop event.

> 
> Nevertheless I already changed the code and built it successfuly. After your
> confirmation, I’ll attach the patch. Thanks!

Yes, let's take a look - thanks!

Comment 6

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Ruth from comment #4)
> However I couldn't reproduce the problem. How can one widget be destroyed in
> mid-air? It always returns to the initial position without any exception.

I'm sorry about this, I should have posted a working example sooner.

STR:
1. Install the latest beta version 2b1 of The Puzzle Piece add-on (https://addons.mozilla.org/firefox/addon/the-puzzle-piece/versions/, it will install with the name Puzzle Toolbars as I'm planning a name change, so that's expected)
2. Enter customize mode
3. Drag a separator into the menu panel
4. Now drag that same separator out of the menu panel into the navigation bar

This will appear in the browser console:
> "[CustomizeMode]" TypeError: draggedWrapper is null
> Stack trace:
> CustomizeMode.prototype._onDragEnd@resource://app/modules/CustomizeMode.jsm:1896:5
> CustomizeMode.prototype._applyDrop@resource://app/modules/CustomizeMode.jsm:1840:5
> CustomizeMode.prototype._onDragDrop@resource://app/modules/CustomizeMode.jsm:1718:7
> CustomizeMode.prototype.handleEvent@resource://app/modules/CustomizeMode.jsm:1442:9

Long story short, since "special widgets" (e.g. separators) aren't allowed by default in the menu panel, the add-on fools CustomizableUI into thinking it's not actually moving a special widget there, by changing its id into something else, let's call it a SuperDuperMegaSpecial Widget id. And when you drag that separator out of the menu panel, the add-on knows that this SuperDuperMegaSpecial widget id shouldn't be used anywhere outside of the menu panel, so it changes it back to a special widget id.

CustomizableUI doesn't actually allow a widget id change, so whenever this happens the add-on is actually destroying the previous widget and forcing the creation of a new one with a new id when it needs to.

Codewise, when dragging the separator out of the menu panel, after releasing the mouse button over the nav bar:
- CustomizeMode._applyDrop() is called
- Everything runs up until the CustomizableUI.addWidgetToArea() call: http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#1820
- This is where the add-on code kicks in: https://github.com/Quicksaver/The-Puzzle-Piece/blob/master/resource/modules/specialWidgets.jsm#L21-L30
- The add-on destroys the SuperDuperMegaSpecial Widget that is "technically still being dragged" and forces the creation of a new special widget in the nav bar
- Afterwards, CustomizeMode._onDragEnd() is called, and it uses an id, of the widget being "moved", that is stored somewhere that is very difficult to access from within the add-on (or at all, is it even possible to access an event from the outside?), and just assumes it will return a valid node, but it won't because this is still the SuperDuperMegaSpecial widget id, which was just destroyed: http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizeMode.jsm#1878

So, while everything "works" as expected, the drag operation never actually fully terminates. I hope this helps with the testing.

(I realize I could just not do this, and move the SuperDuperMegaSpecial widget directly from the menu panel to the nav bar, but that would require at least five times the amount of the current code, just to correctly manage those like CustomizableUI manages special widgets, and I'd rather use CustomizableUI itself for the purpose it was intended than make a copy of it just for this; hence this bug. :) I'm sure I've also tried destroying the SuperDuperMegaSpecial widget after the drag operation terminates to avoid this, but for the life of me I can't remember why that didn't work right now...)

Comment 7

[Ruth]

Attachment: bug1062014.patch

Ok, thanks for the explanation! :)
I managed to reproduce the error and solved the problem adding the check for existence of draggedWrapper. 
Here's my patch.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8504452 [details] [diff] [review]
bug1062014.patch

Review of attachment 8504452 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good! Just a few nits to address.

::: browser/components/customizableui/CustomizeMode.jsm
@@ +1575,5 @@
>        aEvent.dataTransfer.mozGetDataAt(kDragDataTypePrefix + documentId, 0);
>      let draggedWrapper = document.getElementById("wrapper-" + draggedItemId);
>      let targetArea = this._getCustomizableParent(aEvent.currentTarget);
>      let originArea = this._getCustomizableParent(draggedWrapper);
> +     

Nit - trailing whitespace.

@@ +1875,5 @@
>      let draggedItemId =
>        aEvent.dataTransfer.mozGetDataAt(kDragDataTypePrefix + documentId, 0);
>  
>      let draggedWrapper = document.getElementById("wrapper-" + draggedItemId);
> +    if (draggedWrapper){

Nit - space before {

Also, a quick comment above detailing how it's possible that draggedWrapper might be undefined would be good!

@@ +1876,5 @@
>        aEvent.dataTransfer.mozGetDataAt(kDragDataTypePrefix + documentId, 0);
>  
>      let draggedWrapper = document.getElementById("wrapper-" + draggedItemId);
> +    if (draggedWrapper){
> +    	draggedWrapper.hidden = false;

Nit - two space indentation.

Comment 9

[Ruth]

Attachment: bug1062014.patch

Ok! Here's the second version.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8504828 [details] [diff] [review]
bug1062014.patch

Review of attachment 8504828 [details] [diff] [review]:
-----------------------------------------------------------------

There's some trailing whitespace, and I might re-word the comment slightly, but this looks good. I'll make the small corrections and push this for you. Thanks Ruth!

If you want to hack on another bug, check out bug 850709 - that one is very high value!

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Landed in fx-team as:

remote:   https://hg.mozilla.org/integration/fx-team/rev/af04c9b0b7f4

Thanks again, Ruth!

Comment 12

[Ruth]

Thank you very much for your help and feedback, Mike!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/af04c9b0b7f4