1120784 - Page navigation in debugger seems pretty broken

Status: RESOLVED INVALID

(DevTools :: Debugger, defect)

Description

[Shu-yu Guo [:shu]]

While investigating the leak in browser_dbg_breakpoints-break-on-last-line-of-script-on-reload.js
 pointed out by the patch from bug 1084626, I ran into the following leak in SourceActors, though it seems to me that this bug encompasses all actors.

A ThreadActor's actors pool is thread-lifetimed, meaning the pool is freed and the transport connection is closed when the TA goes away, which is basically on outer window destruction (tab closure).

On the other hand, on *inner* window destruction (navigation), the actors pool is persisted. Moreover, the transport layer seems able to deliver packets *across* a navigation. Meaning, if we navigated from P1 -> P2, P1 could have sent a packet, then we navigated to P2, then P2 received the packet. This seems wacko and these packets can refer to actors that reference things from the previous, now-dead inner window.

Example log spew:

DBG-SERVER: Received packet 51: { 
  "to": "conn0.context63",
  "type": "sources"
}
DBG-SERVER: Packet 52 sent from "conn0.context63"
++DOMWINDOW == 32 (0x7f32fbef0c00) [pid = 13538] [serial = 32] [outer = 0x7f3303eb8400]
DBG-SERVER: Packet 53 sent from "conn0.tab2"
***** NAVIGATION ******
DBG-SERVER: Packet 54 sent from "conn0.context63"
-*-*- UserCustomizations (parent): document created: http://example.com/browser/browser/devtools/debugger/test/doc_breakpoints-break-on-last-line-of-script-on-reload.html
-*-*- UserCustomizations (parent): _injectInWindow
-*-*- UserCustomizations (parent): principal status: 0
DBG-SERVER: Received packet 52: { 
  "sources": [
    {
      "actor": "conn0.source65",
      "url": "http://example.com/browser/browser/devtools/debugger/test/code_breakpoints-break-on-last-line-of-script-on-reload.js",
      "addonID": null, 
      "addonPath": null, 
      "isBlackBoxed": false,
      "isPrettyPrinted": false,
      "introductionUrl": null, 
      "introductionType": "scriptElement"
    }
  ],
  "from": "conn0.context63"
}

What the above is supposed to show is that 'conn0.context63' sent a 'sources' packet with a SourceActor 'conn0.source65' across navigation. The 'conn0.source65' SA refers to a source in the navigated-away from page. With the patch in bug 1084626 applied, trying to access it raises a dead object exception.

So 2 issues:

 1. Why are we retaining dead actors across navigations? Are all actors from all pages retained until tab closure or is there cleanup code? Where is the cleanup code? Nick Fitzgerald mentioned a planned navigation lifetime actor pool. That seems to be necessary.

 2. Why can we deliver packets across navigations? That seems to be incorrect.

Comment 1

[Shu-yu Guo [:shu]]

Nick seems to know the code best.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

I don't think packet 52 is actually delivered across navigation. We can see the message:

Packet 52 sent from "conn0.context63"

before navigation occurs, so in the server side everything happens as expected. The log interleaves the client and server messages, and, indeed, the client receives the sources response after navigation, and presumably shortly after that it will receive the tabNavigated packet, too.

I haven't looked into the other bugs yet, but if the issue is that the client then requests one of the old sources, then this is probably a bug. The frontend clears its source caches on navigation (see DebuggerController._onTabNavigated) and the backend, should perhaps do so as well (in TabActor._windowReady). Maybe storing the source actors in a new navigation-lifetime pool would make this easier.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

> then this is probably a bug.

What I meant to say is that, this is probably *the* bug.

Comment 4

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

Panos, Shu: I don't think these bugs are mutually exclusive, and they are probably playing off of each other.

We definitely shouldn't be requesting new sources *before* the navigation.

Source list requests definitely shouldn't cross navigations, although something like attach/detach could.

We need navigation lifetime pool just to be sane about not keeping actors alive almost forever.

SourceActors belong in the navigation lifetime pool.

BreakpointActors belong in the thread lifetime pool.

ObjectActors are trickier: if they are used by the debugger, they belong in the navigation lifetime pool; if used by the console, they belong in the thread lifetime pool and need to be released manually by the console (depending on if it is set to clear logs on reload or not).

Comment 5

[James Long (:jlongster)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> 
> SourceActors belong in the navigation lifetime pool.

Oh, we aren't doing that right then: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/actors/script.js#L5232 (it was like that before the Debugger.Source patch, fwiw)

Comment 6

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

(In reply to James Long (:jlongster) from comment #5)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> > 
> > SourceActors belong in the navigation lifetime pool.
> 
> Oh, we aren't doing that right then:
> https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/
> actors/script.js#L5232 (it was like that before the Debugger.Source patch,
> fwiw)

There is no such thing as a navigation lifetime (yet).

Comment 7

[James Long (:jlongster)]

This is a limitation of dbg-client.jsm. protocol.js fixed this by rejecting all responses from the server to clients that don't exist anymore: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/server/protocol.js#L1109

This would fix this because packets sent to the server should be in order, but we don't want to handle responses to a packet sent from the old page session. Simply dropping them works (in comment 1, we get a packet from an old page session actually respond to it).

I feel like at some point we need to prioritize moving the debugger server to protocol.js...

Comment 8

[James Long (:jlongster)]

Even if we properly handled lifetimes of SourceActors, this could still happen, and the fix I described in comment 7 is really the real fix I think.

Comment 9

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

(In reply to James Long (:jlongster) from comment #8)
> Even if we properly handled lifetimes of SourceActors, this could still
> happen, and the fix I described in comment 7 is really the real fix I think.

There are two issues here: (1) correctness, which comment 7 does indeed seem like the right fix for, and (2) not holding onto actors (and the resources they hold onto in turn) for way longer than need be. For (2), we do need to add the navigation lifetime pool.

Comment 10

[Jason Laster [:jlast]]

Honza, could you see if this is still relevant?

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Shu: is this bug still reproducible today?

Honza

Comment 12

[Jason Laster [:jlast]]

Closing as this is related to how the old client communicated with the server and the links/files referenced have changed significantly with the protocol/actor-refactoring.