Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#88 closed enhancement (fixed)

destroy js errors for LegendPanel and LayerContainer

Reported by: bartvde Owned by: ahocevar
Priority: major Milestone: 0.5
Component: GeoExt.data.LayerStore Version: SVN
Keywords: Cc:
State: Commit

Description

I am running into issues where both the LegendPanel and the LayerContainer are trying to remove stuff from the DOM too late. Something similar is described by Animal on the ExtJS forums here:

http://extjs.com/forum/showthread.php?t=49907

An easy check is to see if an element has a dom property before removing it. I'll attach a patch to do exactly this.

The errors have to do with El.cache and El._flyweights.

Attachments (3)

ticket88.patch (1017 bytes) - added by bartvde 8 years ago.
patch fixing the issue
geoext-88.patch (2.6 KB) - added by ahocevar 8 years ago.
detect map being destroyed and unbind
88.patch (2.4 KB) - added by tschaub 8 years ago.
unbind when destroying

Download all attachments as: .zip

Change History (19)

Changed 8 years ago by bartvde

patch fixing the issue

comment:1 Changed 8 years ago by tschaub

Hey Bart - see also my comments on #86.

Same issue? If you uncomment map.destroy in the LayerContainer tests you can see the problem.

comment:2 Changed 8 years ago by bartvde

Hey Tim, I tried your patch but it did not solve my issue unfortunately.

comment:3 Changed 8 years ago by bartvde

Oh right, Tim's patch to #86 is not supposed to fix this issue, I was misreading.

comment:4 Changed 8 years ago by bartvde

Tim, when running with map.destroy from #86 I am getting:

exception: : object: 'length' is null or not an object

Is this what you mean? The error message is different from what I am getting though.

comment:5 Changed 8 years ago by bartvde

My error message is:

Error: 'undefined' is null or not an object

comment:6 Changed 8 years ago by bartvde

This actually breaks removing elements from the tree and legend. So it is not the correct solution.

comment:7 Changed 8 years ago by ahocevar

Hey Bart, are you currently working on this? If not, I could investigate this a bit further today. Please let me know.

comment:8 Changed 8 years ago by bartvde

Hi Andreas, I haven't looked into it deeper unfortunately and won't be able to do that today, but the problem is I have not been able to create a reproduceable test case outside of that Geonetwork application. If you think you are able to help even without a testcase, then your help would be most welcome. Otherwise I'll try and investigate with the application over the weekend.

comment:9 Changed 8 years ago by tschaub

For a destroy issue that is easy to reproduce, see #90. May or may not turn out to be a duplicate of Bart's issue.

comment:10 Changed 8 years ago by ahocevar

  • Component changed from widgets.tree to data.LayerStore
  • Owner ahocevar deleted
  • State changed from None to Review

I did some research and found a pattern that causes the unload issue:

store.on("remove", function() {
    this.control.destroy();
    this.control = new OpenLayers.Control({layers: this.map.layers});
    this.map.addControl(this.control);
}, this);

This obviously recreates a control with the new set of layers, after a layer was removed. This works well, unless a layer is removed because we are destroying the map, in which case this.map.addControl() is likely to fail.

So as proposed by Tim, a beforedestroy event on the map would be helpful (see http://trac.openlayers.org/ticket/2136). In the meantime, we can check for map.unloadDestroy, which will be null when the map is being destroyed. When we stumble across this, we can unbind the store from the map.

The above patch does this, and adds a proper destroy method also. I tested with OpenGeo's GeoExplorer, which fails on unload without the patch, and unloads fine with the patch.

Please review.

Changed 8 years ago by ahocevar

detect map being destroyed and unbind

comment:11 Changed 8 years ago by bartvde

Andreas, I can confirm your patch works for me. Great work!

The alternative I came up with was to an extra Ext.get before actually removing something from the DOM, but your patch is fixing the root of the problem which is much better.

            if(node) {
                node.un("move", this.onChildMove, this);
                if (Ext.get(node)){
                    node.remove();
                }
            }

Changed 8 years ago by tschaub

unbind when destroying

comment:12 Changed 8 years ago by tschaub

  • Owner set to ahocevar
  • State changed from Review to Commit

This looks good to me. Andreas, the only change I made was to remove the this.control stuff from the test. I don't think that was relevant - let me know if that was wrong.

Please commit.

comment:13 Changed 8 years ago by ahocevar

  • Resolution set to fixed
  • Status changed from new to closed

(In [1085]) Unbind the store when the map is being destroyed. r=tschaub (closes #88)

comment:14 follow-up: Changed 8 years ago by ahocevar

Tim: I committed the version with the this.control stuff in the test and added a comment why it is there.

comment:15 in reply to: ↑ 14 Changed 8 years ago by tschaub

Replying to ahocevar:

Tim: I committed the version with the this.control stuff in the test and added a comment why it is there.

Yeah, I understood where it came from - and thanks for the comment. However, I still would suggest that it adds some confusion if things start failing again.

First this.control is created (where this is window). However, this.control is not added to the map. Then, the "remove" listener increments the count, destroys the control, creates another one, and adds it to the map. The test is set to fail if the count is too high. This part is clear, and if there is a regression, it will be easy to debug. In my opinion, adding the this.control stuff just confuses things.

Did you mean to put in something like the following:

            var control = new OpenLayers.Control();
            map.addControl(control);
            store.on("remove", function() {
                count++;
                control.destroy();
                control = new OpenLayers.Control();
                map.addControl(control);
            });

At least the code above doesn't leave around window.control after the test has run.

Compare how the following fails at r1084:

        function test_map_destroy(t) {
            t.plan(1);
            
            var map = new OpenLayers.Map({div: "mappanel", allOverlays: true});
            var a = new OpenLayers.Layer("a");
            var b = new OpenLayers.Layer("b");
            map.addLayers([a, b]);

            var store = new GeoExt.data.LayerStore({
                map: map
            });
            
            var count = 0;
            store.on("remove", function() {
                count++;
            });
            
            map.removeLayer(a);
            map.destroy();
            
            t.eq(count, 1, "store's remove handler called once");
        }

In my mind, this is a nice clear failure - and a regression would be easy to detect.

The existing test fails at r1084 in a weird way. The t.eq test is never run. Instead the "remove" listener is called and it tries to add a control to a map that has already been destroyed. The unhandled exception is about this.controls being null (which happens in map.destroy()). Looking at the test, I can imagine someone might think the this.control is related. It is not. The map.addControl(control) call is to blame.

Anyway, I don't want the comment to be taken the wrong way. You did a great job of debugging and fixing this. I just think it makes sense to create the simplest tests possible in case things start failing again.

comment:16 Changed 8 years ago by ahocevar

(In [1102]) tschaub is right - the test will also show regressions without doing control stuff in the remove handler. (see #88)

Note: See TracTickets for help on using tickets.