Web Inspector: Uncaught Exception in ProbeManager deleting breakpoint

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Created attachment 244316 [details]
[TEST] Test Case

* SUMMARY
Sometimes I see an uncaught exception when removing a breakpoint. (Just spam creating and removing breakpoints).

* STEPS TO REPRODUCE
1. Inspect attached test case
2. Select resource "foo.min.js"
3. Pretty print the resource
4. Spam setting and removing a breakpoint on lines 4-6. (Reload the page occasionally)
  => exception in inspector

* NOTES
- Phantom breakpoint issues are unrelated and fixed elsewhere.
- Breakpoint not getting hit is unrelated and fixed elsewhere.

* EXCEPTION
[Error] TypeError: undefined is not an object (evaluating 'knownProbeIdentifiers.forEach')
	_breakpointActionsChanged (ProbeManager.js, line 152)
	dispatch (Object.js, line 141)
	dispatchEventToListeners (Object.js, line 156)
	clearActions (Breakpoint.js, line 356)
	removeBreakpoint (DebuggerManager.js, line 403)
	textEditorBreakpointRemoved (SourceCodeTextEditor.js, line 971)
	_documentMouseUp (TextEditor.js, line 1334)
	(anonymous function) ([native code], line 0)

[Radar WebKit Bug Importer]

<rdar://problem/19422299>

[Joseph Pecoraro]

I think this is a JavaScriptCore issue. We have a Map, and we only ever set values of "new Set". Later on, suddenly we get a non-Set object (I've seen random values in my test, the value 1, or undefined) which leads us to a crash. I'll see if I can reduce.

[Joseph Pecoraro]

Made a reduction (attached). Seems to be an issue with Map as values get added/removed.

<script>
var map = new Map;
function Obj(n) { this.n = n; }

map.set(new Obj(0), []);
map.set(new Obj(1), []);
map.set(new Obj(2), []);
map.set(new Obj(3), []);
map.set(new Obj(4), []);
map.set(new Obj(5), []);
map.set(new Obj(6), []);
map.set(new Obj(7), []);

setInterval(function() {
    var newObject1 = new Obj(8);
    var newObject2 = new Obj(9);
    map.set(newObject1, []);
    map.set(newObject2, []);
    setTimeout(function() {
        console.assert(map.get(newObject1).forEach);
        map.delete(newObject1);
        console.assert(map.get(newObject2).forEach);
        map.delete(newObject2);
    }, 50);
}, 100);
</script>

[Joseph Pecoraro]

Created attachment 244360 [details]
[TEST] Reduction

[Joseph Pecoraro]

No ASSERTs in a debug build.

[Joseph Pecoraro]

Created a better reduction with no timeouts. Setting a key/value breaks a different key/value!

map.set(newObject1, []); // set Object1.
console.log(map.get(newObject1)); // Object1 is good.
map.set(newObject2, []); // set Object2.
console.log(map.get(newObject1)); // Object2 is bad!?

[Joseph Pecoraro]

Created attachment 244365 [details]
[TEST] Better Reduction

[Joseph Pecoraro]

(In reply to comment #7)
> Created attachment 244365 [details]
> [TEST] Better Reduction

Looks like MapData::replaceAndPackBackingStore fixes pointer values in the value and string hash maps but forgot about the cell hash maps. Following the pattern for m_cellKeyedTable fixes the issue for me.

[Oliver Hunt]

(In reply to comment #8)
> (In reply to comment #7)
> > Created attachment 244365 [details]
> > [TEST] Better Reduction
> 
> Looks like MapData::replaceAndPackBackingStore fixes pointer values in the
> value and string hash maps but forgot about the cell hash maps. Following
> the pattern for m_cellKeyedTable fixes the issue for me.

Seriously? i am a muppet.

[Joseph Pecoraro]

Created attachment 244368 [details]
[PATCH] Proposed Fix

Suggestions for a more future-proof test welcome.

[Oliver Hunt]

Comment on attachment 244368 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=244368&action=review

> LayoutTests/ChangeLog:5
> +

could you add the radar?

[Joseph Pecoraro]

http://trac.webkit.org/changeset/178224

[David Kilzer (:ddkilzer)]

*** Bug 135879 has been marked as a duplicate of this bug. ***