Closed Bug 697040 Opened 13 years ago Closed 12 years ago

The Script Debugger onNewScript notifications don't always fire

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 13

People

(Reporter: past, Assigned: past)

References

Details

Attachments

(2 files, 2 obsolete files)

STR:

1. Build Nightly from the remote-debug repo:

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug

2. Start Nightly and visit www.mozilla.org (or any other page with scripts).
3. Open the Script Debugger (Tools -> Web Developer -> Script Debugger, or Cmd-Alt-I)
4. Reload the page.
5. Observe the list of scripts in the drop down box and the loaded script in the editor pane. The console output (stdout) will display a line like the following, each time the onNewScript hook is called:

DBG-SERVER: Got a new script:[object Script], url: http://www.mozilla.org/script/1.0/mozilla-input-placeholder.js, startLine: 1, lineCount: 202, strictMode: undefined, function: undefined

It comes from the first line in the new script handler:

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/file/38c048a9a8ec/toolkit/devtools/debugger/server/dbg-script-actors.js#l563

This will be followed shortly thereafter with a protocol notification, like this:

DBG-SERVER: Got: {"from":"conn1.context17","type":"newScript","url":"http://www.mozilla.org/script/1.0/mozilla-pager.js","startLine":1}

6. If you keep reloading the page, at some point it will not display the scripts. The console will not show any output from the onNewScript handler at all.
Jim and Jason, if one of you guys could look into this at some point, I'd appreciate it. As far as I can tell the problem must be in jsdbg2 code, since the hook isn't getting called at all.
Blocks: minotaur
Priority: -- → P2
Okay, I'll take a look at this.
Assignee: nobody → jimb
To get the Script Debugger menu item to appear, you need to set devtools.debugger.enabled to true.
www.mozilla.org refers to http://mozcom-cdn.mozilla.net/org/script/1.0/mozilla.org.js. When I hit reload several times, I can see js::Debugger::slowPathOnNewScript being called to report that script being loaded in a global that has never been passed to js::Debugger::addDebuggee.

Are you sure you have added the new page's debuggee? Here's the interaction after I hit "reload". It seems like we're loading scripts before the debuggee has been attached to.

++DOMWINDOW == 20 (0x7fffd676e078) [serial = 24] [outer = 0x7fffdde45c00]
DBG-SERVER: Got: {
  "from": "conn0.tab3",
  "type": "tabNavigated",
  "url": "http://www.mozilla.org/"
}
DBG-SERVER: Got: {
  "to": "conn0.context6",
  "type": "detach"
}
DBG-SERVER: Got: {
  "type": "detached",
  "from": "conn0.context6"
}
JavaScript strict warning: chrome://browser/content/orion.js, line 5095: reference to undefined property style.tagName
JavaScript strict warning: chrome://browser/content/orion.js, line 5778: reference to undefined property lastChild.ignoreChars
DBG-SERVER: Got: {
  "to": "conn0.tab3",
  "type": "detach"
}
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x8053000C: file /home/jimb/moz/remote-debug/content/base/src/nsGenericElement.cpp, line 5344

Breakpoint 10, js::Debugger::slowPathOnNewScript (cx=0x7fffde0d1fa0, script=0x7fffd5c623d0, compileAndGoGlobal=0x7fffd8e22510) at /home/jimb/moz/remote-debug/js/src/vm/Debugger.cpp:820

Breakpoint 11, js::Debugger::slowPathOnNewScript (cx=0x7fffde0d1fa0, script=0x7fffd5c623d0, compileAndGoGlobal=0x7fffd8e22510) at /home/jimb/moz/remote-debug/js/src/vm/Debugger.cpp:853
(gdb) p compileAndGoGlobal->getDebuggers()
$53 = (js::Vector<js::Debugger*, 0ul, js::SystemAllocPolicy> *) 0x0
(gdb)
If I understand what's going on in toolkit/devtools/debugger/server/dbg-browser-actors.js:

It seems like we don't add the DOMWindowCreated listener until the actor gets an "attach" request. If scripts get loaded between the time we create the BrowserTabActor and the time the client decides to attach to them, those scripts will be missed. That seems to be what's happening in the transcript I show in the previous comment.

I think we really need a way to enumerate the scripts that have been loaded into a given global. Until we have that, we need to be watching onNewScript on windows from the moment they're created --- and that means creating Debugger instances even before the client has expressed any interest in the tab, which ought to be unnecessary in a good design.
(In reply to Jim Blandy :jimb from comment #5)
> If I understand what's going on in
> toolkit/devtools/debugger/server/dbg-browser-actors.js:
> 
> It seems like we don't add the DOMWindowCreated listener until the actor
> gets an "attach" request. If scripts get loaded between the time we create
> the BrowserTabActor and the time the client decides to attach to them, those
> scripts will be missed. That seems to be what's happening in the transcript
> I show in the previous comment.

This is more or less what happens, yes. When DOMWindowCreated is received, the server sends an unsolicited tabNavigated notification to the client, which in turn detaches from the old context and tab, lists the tabs again and attaches to the current tab, and then the thread. The debuggee is added to the thread actor on creation (when attaching to the tab), but is enabled only after attaching to the thread. So any onNewScript calls in this time period will definitely be missed.

For some reason though, I can no longer reproduce this with the 8 patches in my queue. I'll try again with an empty patch queue, soon.

> I think we really need a way to enumerate the scripts that have been loaded
> into a given global. Until we have that, we need to be watching onNewScript
> on windows from the moment they're created --- and that means creating
> Debugger instances even before the client has expressed any interest in the
> tab, which ought to be unnecessary in a good design.

Yes, using getAllScripts() has been my long term plan for this. As an alternative plan, I had tried before to make the server attach to the new window when the DOMWindowCreated event was fired, but I encountered two problems:

- there would be an interval between adding the debuggee to the debugger and enabling the hooks, if the server had to wait for a client attach 
- the client needed to be notified somehow to invalidate its tab/thread front objects and request/attach to new ones (listening for DOMWindowCreated, too, wouldn't work for the remote debugging case)

Since this is apparently a timing issue and not a bug in the engine, I'm happy! When I get back to work on this bug I'll either try your workaround, or use getAllScripts if it's available by then.
Component: Developer Tools → Developer Tools: Debugger
QA Contact: developer.tools → developer.tools.debugger
Marking this resolved invalid, as it was a timing issue. Please re-open if needed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
I'm reopening and taking this to track my attempts at a workaround until getAllScripts lands.
Assignee: jimb → past
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Attached patch WIP (obsolete) — Splinter Review
This is an idea of how this could work, assuming that by the time DOMWindowCreated is fired it's not too late. Also, two other problematic cases I'm trying to fix here are:

- opening the debugger in an empty tab and then loading a page
- reloading a page from a file:// URL

Still not working though.
Status: REOPENED → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Attached patch WIP 2 (obsolete) — Splinter Review
This looks promising. There are a few bugs still, but pages with file:// URLs work (after a refresh), and opening the debugger on an empty tab makes it receive the onNewScript notifications when content loads.

The approach is that basically the responsibility to handle DOMWindowCreated events moves from the tab actor to the root actor. The root actor proactively creates a tab actor for the selected tab, before the client has asked for a list of tabs. Also, the thread (context) actor leaves Debugger.prototype.enabled to true, even before the client attaches. This way, from the moment DOMWindowCreated is received until the debuggee is added to the debugger we don't yield to the event loop, so that when scripts are discovered by SpiderMonkey we are ready to be notified.

The assumption that the client will want to examine the selected tab doesn't seem too safe for the remote debugging case, so I still think that when getAllScripts lands we should back this out and use that instead.
Attachment #593909 - Attachment is obsolete: true
Attachment #594238 - Flags: feedback?(jimb)
Attachment #594238 - Flags: feedback?(dcamp)
Attached patch Working patchSplinter Review
This works now. Pages with file:// URLs work (after a refresh), and opening the debugger on an empty tab makes it receive the onNewScript notifications when content loads.

The workaround is based on the observation that when the debugger is opened, the current tab is the one we're interested in, so it gets special treatment. The root actor proactively creates a tab actor for the selected tab, before the client has asked for a list of tabs. Also, the thread (context) actor leaves Debugger.prototype.enabled to true, even before the client attaches. This way, from the moment DOMWindowCreated is received until the debuggee is added to the debugger we don't yield to the event loop, so that when scripts are discovered by SpiderMonkey we are ready to be notified. The tab location change is handled by two DOMWindowCreated handlers, one to initialize the actor and another to notify the clients. The former must come before the latter in order to avoid yielding to the event loop, so I split them among the capturing and the bubbling phase.

This should of course be backed out when bug 723563 is fixed.

This workaround seems messy, but it works and fixes two important use cases. I'm not sure when bug 676281 is going to land, so I'm asking all of you for review to make sure we're covering all the angles.
Attachment #594238 - Attachment is obsolete: true
Attachment #594238 - Flags: feedback?(jimb)
Attachment #594238 - Flags: feedback?(dcamp)
Attachment #596783 - Flags: review?(rcampbell)
Attachment #596783 - Flags: review?(jimb)
Attachment #596783 - Flags: review?(dcamp)
Attachment #596783 - Flags: feedback?
Attachment #596783 - Flags: feedback?
In other words, this places debuggers on all tabs and iframes from the moment they're created, so that we can catch all their newScript events. This seems like the only thing to do until we can enumerate scripts.
Attachment #596783 - Flags: review?(jimb) → review+
(In reply to Jim Blandy :jimb from comment #12)
> In other words, this places debuggers on all tabs and iframes from the
> moment they're created, so that we can catch all their newScript events.
> This seems like the only thing to do until we can enumerate scripts.

Not on all tabs, only the selected tab (on all windows, but just because this redundancy led to a less invasive patch), because the existing UI can only debug the tab it is in. This wouldn't solve the problem when debugging a remote browser, but I assume we'll be able to enumerate scripts by the time we get there.
Comment on attachment 596783 [details] [diff] [review]
Working patch

Let's make sure we have a followup for removing this when getAllScripts is in place (or add a comment about this to whatever bug we already have for integrating getAllScripts).
Attachment #596783 - Flags: review?(dcamp) → review+
(In reply to Dave Camp (:dcamp) from comment #14)
> Comment on attachment 596783 [details] [diff] [review]
> Working patch
> 
> Let's make sure we have a followup for removing this when getAllScripts is
> in place (or add a comment about this to whatever bug we already have for
> integrating getAllScripts).

I made a note in bug 723563 and I'll post the backout patch there once this one lands.
Comment on attachment 596783 [details] [diff] [review]
Working patch

my only concern with this bug is:

+    // Walk over open browser windows.
+    let e = windowMediator.getEnumerator("navigator:browser");
+    while (e.hasMoreElements()) {
+      let win = e.getNext();

in _preInitTabActor.

It feels a little heavy watching every open browser window when all we're really interested in is the currently-selected tab. But it says "hack" right on the label so if this moves us forward, I guess I'm ok with it.
Attachment #596783 - Flags: review?(rcampbell) → review+
If there are multiple windows, doesn't _preInitTabActor assign this.browser to the selected browser of some random window? (I don't know enough about this to tell why you're walking all windows in the first place -- I assume there's a good reason for that?)
(In reply to Dão Gottwald [:dao] from comment #17)
> If there are multiple windows, doesn't _preInitTabActor assign this.browser
> to the selected browser of some random window? (I don't know enough about
> this to tell why you're walking all windows in the first place -- I assume
> there's a good reason for that?)

I'm explicitly not considering the multiple-windows-open case, since this is a temporary workaround that will be removed ASAP. The only case I want to have working right now, is a single window with the debugger open in the selected tab. This is a trimmed down version of the BRA_onListTabs function, which needs to iterate over all open windows, since it is server code on a potentially remote browser.
try revealed that this patch breaks a GCLI test. It happens to have an open tab with a chrome:// URL, causing the debugger to try to debug a chrome window. I'm attaching the simple change in the patch.
Attachment #599568 - Flags: review?(rcampbell)
Attachment #599568 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/mozilla-central/rev/ccf5cb9f3382
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 13
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: