Opened 7 years ago

Closed 7 years ago

#16002 closed defect (fixed)

dojox.mobile._ContentPaneMixin throws exception

Reported by: Bill Reed Owned by: Eric Durocher
Priority: high Milestone: 1.8.1
Component: DojoX Mobile Version: 1.8.0
Keywords: Cc: cjolif
Blocked By: Blocking:

Description

With Maqetta.org we allow users to dynamically change mobile themes within the html visual editor. When the theme is changed on a page we reparse the dojo widgets, the re-parsing of a dojox.mobile.ContentPane? with a href attribute throws and exception. dojo/parser::parse() error DOMException

parser.js:411

(anonymous function) parser.js:411 _4b8 dojo.js:15 _4b7.then._4c5.then dojo.js:15 lang.extend.otherwise dojo.js:15 _1a.parse parser.js:410 _2246._processWidgets davinci.js:48614 dojo.ready.dojo.addOnLoad._5de dojo.js:15 _5da dojo.js:15 _5d6 dojo.js:15 dojo.ready.dojo.addOnLoad dojo.js:15 _2246._processWidgets davinci.js:48612 _447 dojo.js:5518 _446.then._454.then dojo.js:5628 _2246._processWidgets davinci.js:48611 _2246._setSourceData davinci.js:48587 _2246._continueLoading davinci.js:48464 _2246._setSource davinci.js:48357 ret.withDoc dojo.js:6549 _2246.setSource davinci.js:48207

I have debug the problem dojox.mobile.ProgressIndicator

Here is the flow of what happens When a dojox.mobile._ContentPaneMixin is constructed a instance of a progress indicator is retrieved

constructor: function(){
            // summary:
            //      Creates a new instance of the class.
            // tags:
            //      private
            if(this.prog){
                this._p = ProgressIndicator.getInstance();
            }
        },

The ProgressIndicator.getInstance creates a ProgressIndicator if one does not already exist and assigned it to the global cls._instance

cls.getInstance = function(props){
        if(!cls._instance){
            cls._instance = new cls(props);
        }
        return cls._instance;
    };

dojox.mobile._ContentPaneMixin._setHrefAttr is callled if we have an instences of ProgressIndicator it's dom is appended to the body

            if(this.lazy || !href || href === this._loaded){
                this.lazy = false;
                return null;
            }
            var p = this._p;
            if(p){
                win.body().appendChild(p.domNode);
                p.start();
            }

Once the content is loaded from the href dojox.mobile._ContentPaneMixin._setContentAttr is called which then calls the ProgressIndicator.stop()

        
            this.destroyDescendants();
            if(typeof data === "object"){
                this.containerNode.appendChild(data);
            }else{
                if(this.executeScripts){
                    data = this.execScript(data);
                }
                this.containerNode.innerHTML = data;
            }
            if(this.parseOnLoad){
                var _this = this;
                return Deferred.when(lazyLoadUtils.instantiateLazyWidgets(_this.containerNode), function(){
                    if(_this._p){ _this._p.stop(); }
                    return _this.onLoad();
                });
            }
            if(this._p){ this._p.stop(); }
            return this.onLoad();
        }

in ProgressIndicator.stop the domNode of the progress indicator is removed, but the cls._instance in not set to null

stop: function(){
            // summary:
            //      Stops the spinning of the ProgressIndicator.
            if(this.timer){
                clearInterval(this.timer);
            }
            this.timer = null;
            if(this.removeOnStop && this.domNode && this.domNode.parentNode){
                this.domNode.parentNode.removeChild(this.domNode);
            }
        },

So that the next time the _ContentPaneMixin constructor is call the ProgressIndicator.getInstance(); returns the old instance of ProgressIndecator that is missing the domNode. So that when dojox.mobile._ContentPaneMixin._setHrefAttr and attempts to execute the line

var p = this._p;
            if(p){
                win.body().appendChild(p.domNode);
                p.start();
            }

an exception is thrown

if I add code to dojox.mobile.ProgressIndicator.stop that clears the cls._instance

stop: function(){
            // summary:
            //      Stops the spinning of the ProgressIndicator.
            if(this.timer){
                clearInterval(this.timer);
            }
            this.timer = null;
            if(this.removeOnStop && this.domNode && this.domNode.parentNode){
                this.domNode.parentNode.removeChild(this.domNode);
                cls._instance = null; // wdr
            }
        },

then the exception is resolved.

Attachments (3)

ProgressIndicator.js.patch (434 bytes) - added by Bill Reed 7 years ago.
Possible patch
ProgressIndicator.js.2.patch (415 bytes) - added by Bill Reed 7 years ago.
Better path, add destroy method
ProgressIndicator.js.3.patch (441 bytes) - added by Eric Durocher 7 years ago.
Test if the destroyed instance is the shared instance - Eric Durocher (IBM, CCLA)

Download all attachments as: .zip

Change History (18)

Changed 7 years ago by Bill Reed

Attachment: ProgressIndicator.js.patch added

Possible patch

comment:1 Changed 7 years ago by Bill Reed

I have attached a possible patch, Also here is a link to the Maqetta github issue https://github.com/maqetta/maqetta/issues/3394

comment:2 Changed 7 years ago by bill

Component: GeneralDojoX Mobile
Owner: set to Eric Durocher

comment:3 Changed 7 years ago by Eric Durocher

I don't really see a problem in this code sequence.

The stop method remove the ProgressIndicator's domNode:

                this.domNode.parentNode.removeChild(this.domNode);

Then _ContentPaneMixin re-adds it to the document:

                win.body().appendChild(p.domNode);

That seems correct to me. Also I don't quite understand this:

> ProgressIndicator.getInstance(); returns the old instance of ProgressIndicator that is missing the domNode

What do you mean by "missing"? Is the domNode reference null? It has been removed from its parent, but it should still be a valid node that can be added again to the DOM.

Are you sure that there no other code that would accidentally destroy the ProgressIndicator?

Can you reproduce on a simple sample, outside of Maqetta?

Last edited 7 years ago by Eric Durocher (previous) (diff)

comment:4 in reply to:  3 Changed 7 years ago by Bill Reed

Replying to edurocher:

I don't really see a problem in this code sequence.

The stop method remove the ProgressIndicator's domNode:

                this.domNode.parentNode.removeChild(this.domNode);

Then _ContentPaneMixin re-adds it to the document:

                win.body().appendChild(p.domNode);

That seems correct to me. Also I don't quite understand this:

> ProgressIndicator.getInstance(); returns the old instance of ProgressIndicator that is missing the domNode

What do you mean by "missing"? Is the domNode reference null? It has been removed from its parent, but it should still be a valid node that can be added again to the DOM.

Correct the ProgressIndicator?.getInstrance() returns the ProgressIndicator? object p the object does not have a domNode property, p.domNode must have been delete somewhere.

Are you sure that there no other code that would accidentally destroy the ProgressIndicator?

We do not have any code that would delete the progress indicator. We do destroy and recreate the ContentPane?

Can you reproduce on a simple sample, outside of Maqetta?

I have not tried to recreate outside of Maqetta. I was hoping you might have an idea where the domNode is being destroyed. I will attempt to recreate outside of maqetta.

comment:5 Changed 7 years ago by Eric Durocher

Thanks. No, sorry I don't see any call (at least in dojox/mobile) that could possibly destroy the indicator (especially since it is added directly to the body, so it could not be destroyed as part of a destroyDescendants for example).

comment:6 Changed 7 years ago by Bill Reed

We do destroy all the dojo widgets in the registry before we re-parse the page so we do not get widgetid already defined errors. It looks like when the ContentPane? or the progress bar are destoryed they do not delete the cls._instance

Here is an HTML test file that reproduces the error

<!DOCTYPE html>
<html>
<head>
	<meta http-equiv="Content-type" content="text/html; charset=utf-8"/>
	<meta name="viewport" content="width=device-width,initial-scale=1,maximum-scale=1,minimum-scale=1,user-scalable=no"/>
	<meta name="apple-mobile-web-app-capable" content="yes"/>
	<title>ContentPane</title>

	<script type="text/javascript" src="../deviceTheme.js" data-dojo-config="mblThemeFiles: ['base']"></script>
	<script type="text/javascript" src="../../../dojo/dojo.js" data-dojo-config="async: true, parseOnLoad: true"></script>

	<script type="text/javascript">
		require([
			"dojo/dom-construct",
			"dojo/ready",
			"dijit/registry",
			"dojox/mobile/parser",
			"dojox/mobile",
			"dojox/mobile/compat",
			"dojox/mobile/ContentPane"
		], function(domConstruct, ready, registry, parser, mobile,compat,ContentPane){
			ready(function(){
				window.setTimeout(function(){
					dijit.registry.toArray().forEach(function(w){
                 		w.destroy();             
         		 	});
					var cp = new ContentPane({id:"pane2", href:"data/fragment2.html"},'div1');
				},2000);
			});
		});
	</script>
</head>
<body style="visibility:hidden;">
 <div id='div1'>
		<div id="pane2" data-dojo-type="dojox.mobile.ContentPane"
		     href="data/fragment1.html"></div>


	</div>
</div>
</body>
</html>

comment:7 Changed 7 years ago by Bill Reed

A better fix to to add a destroy() method to ProgressIndicator

I still need to test this fix inside Maqetta but I bet it will work

Last edited 7 years ago by Bill Reed (previous) (diff)

Changed 7 years ago by Bill Reed

Better path, add destroy method

comment:8 Changed 7 years ago by Bill Reed

I tested the above patch and it does fix the issue on Maqetta

comment:9 Changed 7 years ago by Eric Durocher

Definitely better yes, but there is no reason to clear the singleton if another ProgressIndicator instance is destroyed, so we should test that the destroyed instance is actually the shared instance. I am attaching a modified patch, could you also test this in Maqetta please? thanks.

Changed 7 years ago by Eric Durocher

Test if the destroyed instance is the shared instance - Eric Durocher (IBM, CCLA)

comment:10 in reply to:  9 Changed 7 years ago by Bill Reed

Replying to edurocher:

Definitely better yes, but there is no reason to clear the singleton if another ProgressIndicator instance is destroyed, so we should test that the destroyed instance is actually the shared instance. I am attaching a modified patch, could you also test this in Maqetta please? thanks.

This fix works on Maqetta.

comment:11 Changed 7 years ago by Eric Durocher

Cc: cjolif added
Priority: undecidedhigh

OK, thanks for the test, then for me this patch 3 is OK to commit.

comment:12 in reply to:  11 Changed 7 years ago by Bill Reed

Replying to edurocher:

OK, thanks for the test, then for me this patch 3 is OK to commit.

It works for us.

comment:13 Changed 7 years ago by cjolif

In [29704]:

refs #16002. Thanks Eric Durocher (IBM, CCLA).

comment:14 Changed 7 years ago by cjolif

Milestone: tbd1.8.1

comment:15 Changed 7 years ago by cjolif

Resolution: fixed
Status: newclosed

In [29705]:

fixes #16002. Thanks Eric Durocher (IBM, CCLA).

Note: See TracTickets for help on using tickets.