Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16199 closed defect (fixed)

dojox/mobile/ListItem problem on iOS6 breaking the todoApp demo

Reported by: Ed Chatelain Owned by: Eric Durocher
Priority: undecided Milestone: 1.8.2
Component: DojoX Mobile Version: 1.8.1
Keywords: ios6 Cc:
Blocked By: Blocking:

Description

dojox/mobile/ListItem problem on iOS6 breaking the todoApp demo.

In some cases a ListItem? can not be clicked on iOS6, (then after waiting for about a minute the ListItem? will become clickable).

You can see the problem on iOS6 by going here: http://archive.dojotoolkit.org/nightly/checkout/demos/todoApp/demo.html Then select and item, and try to click on "Show More"

Or you can go here: http://archive.dojotoolkit.org/nightly/checkout/dojox/app/tests/scrollableTestApp/#Scrollable1 Then after the list is displayed, click the "Back" button, then try to click any of the list items.

Attachments (4)

patch16199.patch (1.6 KB) - added by Adrian Vasiliu 7 years ago.
Protect ListItem and _ItemBase against NPE when getParent() returns null. (Most methods were already protected, but not all.) - Adrian Vasiliu, IBM, CCLA
patch16199-ios6-only.patch (2.9 KB) - added by Adrian Vasiliu 7 years ago.
On iOS >= 6, protect ListItem and _ItemBase against NPE when getParent() returns null. (Most methods were already protected, but not all.) - Adrian Vasiliu, IBM, CCLA
patch16199-new.patch (1.6 KB) - added by Adrian Vasiliu 7 years ago.
Workaround of iOS6 bug: overridden _ItemBase.getParent (without the patch, on iOS6 it returns null in some dojox/app+dojox/mvc context) - Adrian Vasiliu, IBM, CCLA
patch16199-revert.patch (1.1 KB) - added by Adrian Vasiliu 7 years ago.
Now that #16396 is fixed in Dijit (both trunk and 1.8 branch), we can remove the workaround in _ItemBase. - Adrian Vasiliu, IBM, CCLA

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by Adrian Vasiliu

Reproduced in trunk, 1.8, 1.8.1, with iOS 6 (6.0) only, on both real devices and simulator. Did not reproduce with Safari 6.0.1 on Mac OS X 10.8.2, nor with Safari 5.1.7 on Win7.

The bug hurts somehow erratically, but more often (nearly always) after clearing browser's cache than after a simple reload.

As far as I can tell, the main cause of the issue is a bug in the Javascript engine of Mobile Safari in iOS6, for which I attach here a workaround as a ListItem patch (more exactly two variants: patch16199.patch and patch16199-ios6-only.patch). I'll put in a separate comment details about what makes me think it is a Safari bug, and about the workaround proposal.

Now, this is not enough for fixing todoApp. According to my testing (confirmed by Ed), applying the patch submitted for #16133 is also needed (otherwise, the testing scenario: load, touch "go to the swimming pool", "show more", "back" leads to list items that don't have their labels anymore).

So the best fix we currently have in hands is the combination of

  1. the patch for #16133 plus
  2. the ListItem patch attached here.

This fixes both todoApp and the dojox/app test.

Changed 7 years ago by Adrian Vasiliu

Attachment: patch16199.patch added

Protect ListItem and _ItemBase against NPE when getParent() returns null. (Most methods were already protected, but not all.) - Adrian Vasiliu, IBM, CCLA

Changed 7 years ago by Adrian Vasiliu

Attachment: patch16199-ios6-only.patch added

On iOS >= 6, protect ListItem and _ItemBase against NPE when getParent() returns null. (Most methods were already protected, but not all.) - Adrian Vasiliu, IBM, CCLA

comment:2 Changed 7 years ago by Adrian Vasiliu

Explanation about the two versions of the patch: most methods of _ItemBase and ListItem are already protected against the case getParent() returns null, so adding the same protection in the remaining methods sounds logic. This is patch16199.patch (regardless of the browser).

However, we may not want to extend that to methods that weren't protected, since they may have made on purpose the assumption they are only called when getParent() is not null, and adding the protection would hide that the assumption is broken in other situations than the issue in todoApp and on other platforms than iOS6. Hence the variant patch16199-ios6-only.patch (which protects only on ios >= 6).

For instance, before the patch, using a ListItem as a top-level widget (despite the doc saying it should be inside one of our list widgets) leads to an NPE in ListItem._onTouchStart on all touch-enabled browsers. Adding the protection as in patch.16199.patch would hide it, so maybe it's better to only do it for iOS6 where we do need it for fixing the todoApp issue.

On the other side, telling the user the ListItem is improperly used (as top-level widget) through an NPE from an event handler is not the best possible error feedback... Also, the protection only on ios6 makes the patch (and the code behavior) a bit more complex. And it might look surprising that we protect against null parent for all browsers in some methods while in other methods only for ios6.

Overall, my own take is to go for the simple version (patch16199.patch). Anyway, both work.

Last edited 7 years ago by Adrian Vasiliu (previous) (diff)

comment:3 Changed 7 years ago by Adrian Vasiliu

And here are details about what makes me think a Safari bug hurts here.

The first line in _ItemBase._onTouchStart is the following:

if(this.getParent().isEditing || this.onTouchStart(e) === false){ return; } // user's touchStart action

Under suspicion that getParent() might return null thus leading to an NPE, I've tried to run the app connected to WebInspector and check the console for exceptions. But, guess what, in this configuration the bug doesn't hurt (and the console shows nothing). I never reproduce the issue with either the iPhone or the iPhone simulator connected to Safari 6's webinspector on the Mac - there is no Safari 6 on Windows, so the only way to get the console is to use the Mac. And connecting the console after the bug has hurt doesn't show anything.

Moreover, even if I do not connect to WebInspector, the bug also vanishes after the simple addition of console.log (in particular in dijit/registry.getEnclosingWidget, which is called by _WidgetBase.getParent)!

Hence, instead of relying on console.log for debugging, I collected debug info into a global variable (initialized once per app life time). After running the app till the bug hurts (touching the "Show More" item has no effect), I connect the debugger and inspect the global variable. This way I could get confirmation that, when the bug hurts (and only in this case), getParent returns null.

The strongest sign (although not a formal proof) that this is a Mobile Safari bug is the following: I modified _ItemBase this way:

_myGetParent: function(){
	var node = this.domNode.parentNode;
	while(node){
		var id = node.getAttribute && node.getAttribute("widgetId");
		if(id){
			// direct access to dijit/registry's internal hash
			return registry._hash[id]; 
		}
		node = node.parentNode;
	}
	return null;
},

_onTouchStart: function(e){
	var parent = this.getParent();
	if(!parent){
		logTxt += " getParent null for this.id: " + this.id;
		var myParent = this._myGetParent();
		logTxt += " my parent : " + myParent;
		if(myParent) logTxt += " id: " + myParent.id;
		parent = this.getParent(); // still the same value?
		logTxt += " getParent() called again: " + parent;
	}
	...
},

That is, when getParent returns null, I call my own version of getParent which is a merge of the code from _WidgetBase.getParent (this calls dijit/registry.getEnclosingWidget(this.domNode.parentNode)) and from dijit/registry.getEnclosingWidget itself.

Checking the global logTxt in the console after reproducing the bug, it shows that the original getParent consistently returns null (in both calls in the code above), while my variant, supposed to produce a similar result, succeeds in finding the parent widget!

I have also checked:

  • that the parent widget is indeed previously added to registry._hash, and is not removed afterwards
  • I've printed the content of this.getParent and registry.getEnclosingWidget to be sure their code is the expected one (yes it is)
  • I've added some additional similar debugging code in registry.getEnclosingWidget itself, and this showed that this is the function which executes when _ItemBase._onTouchStart calls getParent, but as soon as I added enough debugging code to understand what happens... the app goes into the state where the bug doesn't hurt anymore...

Extracting a minimal sample to reproduce wasn't possible: the context matters...

All in one, knowing that Safari Mobile in iOS6 clames performance improvements of the javascript engine... these facts seem somehow consistent with the hypothesis that an engine optimization went a bit too far... (which reminds the old times of the childhood of the Java just-in-time compiler, when rewriting code in an equivalent manner was the way to go to workaround Sun's JVM bugs).

Of course, we'll test again when iOS 6.0.1 will be out...

Last edited 7 years ago by Adrian Vasiliu (previous) (diff)

comment:4 Changed 7 years ago by cjolif

Milestone: tbd1.8.2

comment:5 Changed 7 years ago by cjolif

Keywords: ios6 added

comment:6 Changed 7 years ago by Eric Durocher

Have you tried to only replace (or complement) this.getParent() with your variant, which seems to always work? Maybe this makes #16133 unnecessary? (From what I understand, 16133 is necessary because the protection tests just avoid an exception, but you still have to go through another code path so that the events are connected...)

comment:7 Changed 7 years ago by Adrian Vasiliu

Well, wouldn't that be an even less satisfactory workaround? This implies making assumptions about the internal implementation of dijit/registry... Just wondering.

comment:8 in reply to:  7 Changed 7 years ago by Eric Durocher

What I am a bit concerned about is that, with the protection only, the code enters a state where it was never supposed to be before, and (if I understand correctly), the #16133 patch is a workaround to support this new case, but it still seems quite risky (who knows what else may fail after that?) So if we have a way to make sure getParent() -- or a variant -- works correctly, it seems safer to me. And it would also prove that we have found the real cause of the problem. I think it is at least worth testing...

comment:9 Changed 7 years ago by Adrian Vasiliu

Well, if we only use a variant of getParent() in our own code this wouldn't be enough for avoiding the null parent for any code calling the original getParent() on a list item. Hence, if we want to go the way you suggest, I would think we need something like that:

getParent: function(){
  if(has("iphone" >= 6){ // workaround for strange behavior on iOS 6 
    var node = this.domNode.parentNode;
    while(node){
      var id = node.getAttribute && node.getAttribute("widgetId");
      if(id){
        // direct access to dijit/registry's internal hash
        return registry._hash[id]; 
      }
      node = node.parentNode;
    }
    return null;
  }else{
    return this.inherited(arguments);
  }
},

Is an override as this one (plus the #16133 patch) what deserves testing? (just to be sure before going through the tests).

comment:10 Changed 7 years ago by Adrian Vasiliu

It was definitely a good idea to further dig into it, thanks Eric! So it goes this way: I have tested both variants: a) using the "custom" getParent only at the place where the NPE hurts (in _ItemBase._onTouchStart), and b) using an override of getParent such that all calls of getParent benefit from the workaround of the iOS6 bug. The first variant does NOT do the trick alone (without the additional patch of #16133). But the second does it, without the additional patch of #16133!

Trying to understand what's going on here, I have tracked all the calls of getParent for the exact instance of item ("Show More...") for which the bug hurts, and this clarified the picture (finally!). So here it is:

There are 3 calls of getParent for the "Show More..." item when todoApp transitions to the view which contains this item (= when you touch "Go to the swimming pool"). All these calls are already protected against a null parent (while the call in _onTouchStart is not, but this happens only later, when you touch the Show More item itself). So, with getParent returning null, there is no NPE for these calls. But the null parent hurts in the following way: _ItemBase.buildRendering does

this._isOnLine = this.inheritParams();

and the code of inheritParams ends with:

return !!parent;

That is, the impact of the parent being (wrongly) null is that the internal flag _isOnLine is false after item's buildRendering. Now, later, inheritParams is called again in _ItemBase.startup:

if(!this._isOnLine){
  this.inheritParams();
}

so inheritParams does get called in fine. But in the dojox/mvc context we have calls of the setters of item's properties at a time point which is between the buildRendering() and startup() calls. Differently, on other browsers than iOS6 (ex. on Chrome), getParent is not null already at the very first call (from buildRendering()), thus _isOnLine becomes true already at this time point. And this makes the difference...

From that we can better understand why the patch of #16133 was also a workaround: the job which is not done when parameters are set during buildRendering is done after startup. But with the overridden _ItemBase.getParent this is no longer necessary.

All in one, the new patch proposal is the one in patch16199-new.patch.

Last edited 7 years ago by Adrian Vasiliu (previous) (diff)

Changed 7 years ago by Adrian Vasiliu

Attachment: patch16199-new.patch added

Workaround of iOS6 bug: overridden _ItemBase.getParent (without the patch, on iOS6 it returns null in some dojox/app+dojox/mvc context) - Adrian Vasiliu, IBM, CCLA

comment:11 Changed 7 years ago by cjolif

Resolution: fixed
Status: newclosed

In [29867]:

fixes #16199, #16202. Fixes remaining iOS6 issues including one regression. Thanks Adrian Vasiliu (IBM, CCLA). !strict

comment:12 Changed 7 years ago by cjolif

In [29868]:

fixes #16199, #16202. Fixes remaining iOS6 issues including one regression (backport to 1.8). Thanks Adrian Vasiliu (IBM, CCLA). !strict

comment:13 Changed 7 years ago by Adrian Vasiliu

iOS 6.0.1 is now out, and at a quick testing it appears the fix on our side is still useful and still works. (Without our override of getParent in _ItemBase, the misbehavior hurts again).

comment:14 Changed 7 years ago by Adrian Vasiliu

The same holds for Safari included in iOS 6.1 beta (tested using the simulator).

comment:15 Changed 7 years ago by Eric Durocher

Type: defecttask

The same bug seems to hit the SpinWheel widget, so maybe a more general solution shoudl be found? That is, a way to fix getParent() in all cases?

comment:16 Changed 7 years ago by Eric Durocher

Type: taskdefect

(accidentally changed type to task, reverting to defect)

comment:17 Changed 7 years ago by Eric Durocher

Created #16396 to see if this should be fixed more generally in dijit.

Changed 7 years ago by Adrian Vasiliu

Attachment: patch16199-revert.patch added

Now that #16396 is fixed in Dijit (both trunk and 1.8 branch), we can remove the workaround in _ItemBase. - Adrian Vasiliu, IBM, CCLA

comment:18 Changed 7 years ago by cjolif

In [30065]:

Refs #16199. Now that #16396 is fixed in Dijit (both trunk and 1.8 branch), we can remove the workaround in _ItemBase. - Adrian Vasiliu, IBM, CCLA. !strict

Note: See TracTickets for help on using tickets.