iOS 8 beta 5 Set forEach sometimes triggers wrong number of times

[Ashley Gullen]

In our HTML5 game engine we use Sets when supported, and the latest iOS 8 betas have turned up a strange issue that does not happen in any other browser that supports Set: sometimes Set forEach will fire its callback a different number of times to the Set.size property.

I can only reproduce this in a real-world example of our engine, which is pretty complex: http://www.scirra.com/labs/bugs/sbtestbug2/

After about a minute or so, the game will alert a message like "Set forEach mismatch: iterated 7 times when Set size returns 6".

The code in question is below. ObjectSet_.update_cache aims to dump the contents of the Set in to the array this.values_cache.

var current_arr = null;
	var current_index = 0;
	
	function set_append_to_arr(x)
	{
		current_arr[current_index++] = x;
	};
	
	ObjectSet_.prototype.update_cache = function ()
	{
		if (this.cache_valid)
			return;

		if (supports_set)
		{
			this.values_cache.length = this.s["size"];
			
			current_arr = this.values_cache;
			current_index = 0;
			
			this.s["forEach"](set_append_to_arr);
			
			// CHECK FOR THE BUG
			if (current_index !== this.s["size"])
			{
				alert("Set forEach mismatch: iterated " + current_index + " times when Set size returns " + this.s["size"]);
			}
			
			current_arr = null;
			current_index = 0;
		}
		else
		{
			// (unrelated workaround when Set not supported)
		}
		
		// Cache now up to date
		this.cache_valid = true;
	};

I tried to reproduce this in a standalone example trying to emulate what our engine does, but it does not reproduce. For what it's worth, the example that appears to demonstrate everything working is here: http://www.scirra.com/labs/bugs/testsetforeach.html

I know it's difficult to investigate bugs without a standalone repro, but in this case I can't make one outside of the actual game engine, but it does reproduce every time here.

[Radar WebKit Bug Importer]

<rdar://problem/18014274>

[Oliver Hunt]

What happens if you replace forEach with an equivalent:

Map.prototype.forEach = function forEach(fn /* , thisArg */) {
    for (var {key, value} of this)
        fn.call(arguments[1], key, value /*, this -- whoops should jsc be passing this? eep */)
}

(e.g. do we screw up general case iteration, or is this something specific to foreach's internal implementation? - also if it does work that would provide something of a workaround.  I'm also wondering if you're mutating the map while iterating?)

[Oliver Hunt]

(In reply to comment #2)
> What happens if you replace forEach with an equivalent:
> 
> Map.prototype.forEach = function forEach(fn /* , thisArg */) {
>     for (var {key, value} of this)
>         fn.call(arguments[1], key, value /*, this -- whoops should jsc be passing this? eep */)
> }
> 
> (e.g. do we screw up general case iteration, or is this something specific to foreach's internal implementation? - also if it does work that would provide something of a workaround.  I'm also wondering if you're mutating the map while iterating?)

Or the equivalent for set:
Set.prototype.forEach = function forEach(fn /* , thisArg */) {
    for (var value of this)
        fn.call(arguments[1], value /*, this -- whoops should jsc be passing this? eep */)
}

[Oliver Hunt]

Some horrible kludging makes me think no mutation during iteration is occurring. Presumably we're messing up our tracking of sizes somehow. Ick.

[Oliver Hunt]

Although i was only testing Set - is the dialog that pops up lying?

[Oliver Hunt]

Ok, this must be related to our gc compression of the backing store.  Hurgh.

[Ashley Gullen]

New test cases:

http://www.scirra.com/labs/bugs/sbtestbug3/: replaces the following line

this.s["forEach"](set_append_to_arr);

with

for (var v of this.s)
	set_append_to_arr(v);

consequence: early on alerts "Set forEach mismatch: iterated 4 times when Set size returns 3", always reproduces
	
http://www.scirra.com/labs/bugs/sbtestbug4/: same as sbtestbug2, but prepends the javascript code with

Set.prototype.forEach = function forEach(fn) {
    for (var value of this)
        fn(value);
};

consequence: early on (but later than sbtestbug3) alerts "Set forEach mismatch: iterated 14 times when Set size returns 13", always reproduces (but observed with different numbers, like 13 / 12), happens repeatedly afterwards with numbers like 15 / 14, 16 / 15, 17 / 16, after several alerts sometimes is two apart (16 / 14, 18 / 16)

It looks like the first occurrence is always off-by-one, with forEach triggering one more time than the set size reports. Since it also happens with for-of loops it doesn't appear limited to forEach.

[Ashley Gullen]

To address other comments:

- these cases only test Set - Map is not used at all so I'm afraid I can't comment if it is also affected

- the Set is never mutated whilst iterating, if you look at the code example its a pretty trivial way to dump the contents in to an array.

[Oliver Hunt]

(In reply to comment #8)
> To address other comments:
> 
> - these cases only test Set - Map is not used at all so I'm afraid I can't comment if it is also affected
> 
> - the Set is never mutated whilst iterating, if you look at the code example its a pretty trivial way to dump the contents in to an array.

Yeah, i'm currently investigating our gc copy+pack pass my assumption is that it's responsible for messing everything up

[Ashley Gullen]

A new test case: http://www.scirra.com/labs/bugs/sbtestbug5/

This case is based off sbtestbug3, but additionally counts how many of the iterated items are reported as being in the Set using Set.has(). Here's the relevant code:

			for (var v of this.s)
				set_append_to_arr(v);
			
			if (current_index !== this.s["size"])
			{
				// Count how many items in the array are reported as being in the Set
				var i, count = 0;
				
				for (i = 0; i < current_index; ++i)
				{
					if (this.s["has"](this.values_cache[i]))
						++count;
				}
				
				alert("Set for-of mismatch: iterated " + current_index + " times when Set size returns " + this.s["size"] + ". " + count + " of the iterated items are reported as being in the Set.");
			}

Interestingly, I get the following message on iOS 8 beta 5:

"Set for-of mismatch: iterated 4 times for when Set size returns 3. 2 of the iterated items are reported as being in the Set."

This is curious since I was expecting the count to be either 4 (for-of was correct, Set.size was incorrect) or 3 (for-of was incorrect, Set.size was correct). But the number of iterated items reported as being in the set is yet another different value. So neither value is correct, the internal structure of the Set must be invalid or corrupt or something.

[Ashley Gullen]

Still reproduces on iOS 8 release... I think disabling the feature would have been better than shipping a broken one... bet this will cause some devs to tear their hair out wondering what could possibly be wrong!

[Ashley Gullen]

Still reproduces on iOS 8.1.1!

[Ashley Gullen]

Still reproduces on iOS 8.2!

[Oliver Hunt]

huh, this is super confusing, Are you seeing this in desktop safari or a nightly?

[Ashley Gullen]

Yes, the problem described in comment 10 reproduces identically in Safari 8.0.3 running on OS X 10.10.2.

[Oliver Hunt]

(In reply to comment #15)
> Yes, the problem described in comment 10 reproduces identically in Safari
> 8.0.3 running on OS X 10.10.2.

Could you please verify on a nightly (nightly.webkit.org) i can't report on my machine any more :(

[David Kilzer (:ddkilzer)]

Use bisect-builds tool to find where this progressed.

[Ashley Gullen]

Huh, can't reproduce on WebKit Nightly r181469 on OS X 10.10.2. I tested every URL in this comment thread and they all seem to work OK. Perhaps a patch for a different bug fixed this?

[Oliver Hunt]

(In reply to comment #18)
> Huh, can't reproduce on WebKit Nightly r181469 on OS X 10.10.2. I tested
> every URL in this comment thread and they all seem to work OK. Perhaps a
> patch for a different bug fixed this?

No that makes sense, i felt we had fixed this, but clearly the fix slipped through the cracks of the release process. Next step working out which revision fixed it.

If you wanted to be super helpful/insane you could bisect the nightlies and find the revision that fixed it.

I may be able to do so later, but i'm unsure when i will find time (I technically no longer work on webkit)

[Ashley Gullen]

TBH I'm just happy it's working now, I won't be looking up the relevant revision and am OK with this being marked fixed now. I'd ask if you know when the fix will be in a Safari release but I've never been told anything useful when I ask that :P

[David Kilzer (:ddkilzer)]

Dupe of:

Bug 140279: Web Inspector: Uncaught Exception in ProbeManager deleting breakpoint
​<https://bugs.webkit.org/show_bug.cgi?id=140279>

Fixed by:  <http://trac.webkit.org/changeset/178224>

*** This bug has been marked as a duplicate of bug 140279 ***