935373 - Debugger should send a notification when a source is garbage collected

Status: RESOLVED INVALID

(DevTools :: Debugger, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Just as we have "newSource" notifications, we should send out "endSource" notifications after a source is gc'd.

We could do this by creating a weak map on the server that periodically polls the keys and compares them to the last set of keys it got. Any that are missing in the new set should have "endSource" notifications sent out.

This would allow us to show "temporary" evals which don't introduce new global functions, but have them disappear from the sources list after some amount of time.

Comment 1

[Rob Campbell [:rc] (:robcee)]

setting status to assigned and calling it P3. Adjust to taste.

Comment 2

[Dave Camp (:dcamp)]

You can't iterate a WeakMap though.  JS goes through great pains to make GC unobservable.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Dave Camp (:dcamp) from comment #2)
> You can't iterate a WeakMap though.  JS goes through great pains to make GC
> unobservable.

There is a way: http://mxr.mozilla.org/mozilla-central/search?string=nondeterministicGetWeakMapKeys

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I'm unassigning myself because I won't be working on this anytime soon.

Comment 5

[Dave Camp (:dcamp)]

Well would you look at that!  Neat.

Comment 6

[Dave Camp (:dcamp)]

That should be used for testing only, though.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I guess we can just keep all the sources we found from the last call to findScripts() in one set and create a new set with the sources from calling findScripts() again and then compare the two sets. We'd have to make sure we empty the old set afterwards and don't hold on to these temporary sources forever, of course.

Hopefully keeping the sources in a set won't make them keep showing up in subsequent calls to findScripts() because we are keeping GC from taking them...

Comment 8

[James Long (:jlongster)]

The dbg-source bug is now strictly about the big transition to use actors instead of urls everywhere. These bugs can be a follow-up.

Comment 9

[Eddy Bruel [:ejpbruel]]

I strongly suspect that fixing this bug would also help in fixing bug 893696.

Comment 10

[James Long (:jlongster)]

Hm, I forgot about this bug, and in bug 1106695 we made the debugger keep all the scripts alive. We need that so that `findScripts` is deterministic, which from tons of real-world experience was a real pain when it wasn't.

I think we should close this bug as invalid, and if fitzgen had something in mind about eval scripts specifically we can file a new one to figure out what to do about thouse. As for bug 893696, I'm not sure it's entirely valid anymore. With the new Debugger.Source patch, it queries the source object directly for the source contents, so it should always get new text on refresh. There are probably cases where the frontend is caching things when it shouldn't, so we should make it more clear what happens when the text is updated in various scenarios, but I don't think it directly concerns this bug. (for example: multiple eval sources with the same sourceURL will not be updated in the debugger right now, not because of a GC issue but because the frontend hard cached the first contents)

Comment 11

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] from comment #0)
> Just as we have "newSource" notifications, we should send out "endSource"
> notifications after a source is gc'd.
> 
> We could do this by creating a weak map on the server that periodically
> polls the keys and compares them to the last set of keys it got. Any that
> are missing in the new set should have "endSource" notifications sent out.
> 
> This would allow us to show "temporary" evals which don't introduce new
> global functions, but have them disappear from the sources list after some
> amount of time.

The way I would prefer to address use cases like this is:

1) Clearly describe the user-visible behavior we want

2) Consider options for implementing that behavior

rather than suggesting a single problematic interface, with the feature described vaguely. In many cases (including this one, it seems to me) there is a way to solve the problem without introducing even more GC-sensitive APIs.

I know Nick has been burned by GC-sensitive behavior since he posted this bug, and would approach things differently now, but I want to point out the problematic pattern.