1042976 - TEST-UNEXPECTED-FAIL | page-mod-debugger-post/main.testDebugger || TypeError: debuggee.runDebuggerStatement is not a function

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Tomislav Jovanovic :zombie]

nothing that landed recently in jetpack looks like it touched debugger stuff, so this is probably a devtools issue (from bug 897567 maybe?), though Wes informs me the fx-team tree is green atm..

https://tbpl.mozilla.org/php/getParsedLog.php?id=44459560&tree=Jetpack

Comment 1

[Dave Townsend [:mossop]]

Erik, you wrote this test. Any ideas what might have broken it?

Comment 2

[Tomislav Jovanovic :zombie]

note: although it looks like this started failing between two of my pushes yesterday, it's totally unrelated. the cause is somewhere in the yesterday's Nightly (i re-triggered a few test runs after it was built, and they fail in the same way).

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #1)
> Erik, you wrote this test. Any ideas what might have broken it?

Something in the devtools code base probably changed, I may not be the best person to look in to this.

If you want me to though I will.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1565

The bug was caused by set directly the function in `unsafeWindow`; using `exportFunction` instead fix the issue.

Comment 6

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0efe04f0526d15f45caf7775c3e9d7791c71b289
Bug 1042976 - TypeError: debuggee.runDebuggerStatement is not a function

- Used `exportFunction` in test addons

https://github.com/mozilla/addon-sdk/commit/55efdef507c2023ba87268e8ff1726f6237328af
Merge pull request #1565 from ZER0/runDebuggerStatement/1042976

fix Bug 1042976 - TypeError: debuggee.runDebuggerStatement is not a function r=@jsantell

Comment 7

[Dave Townsend [:mossop]]

This test is just timing out now

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Dave Townsend [:mossop] from comment #7)
> This test is just timing out now

This started failing on this push of checkin-needed bugs to fx-team: https://tbpl.mozilla.org/?tree=Fx-Team&jobname=jetpack&showall=1&rev=fea2a90e2d22

Gonna guess it's bug 1039952.

Comment 9

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

It might be related, as bug 1039952 did change the way that debuggee globals are managed by the debugger, which includes the addon debugger. I'm not sure what these tests are, or where exactly the test is located. Those files linked don't tell me much...

Mossop, you wrote a lot of the initial addon debugger code surrounding finding addon debuggee globals, can you take a look at [0] and see if there is anything missing from the original logic?

[0] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#1492

Comment 10

[Dave Townsend [:mossop]]

This test isn't part of the add-on debugger, it's page-mods showing up in the regular webpage debugger I think, that was Erik's work. Erik, how does the page-mod globals get added as debuggees?

Comment 11

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

In that case, for finding all debuggee globals, we use the parent's |windows| getter:

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#551
http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#635

And for new globals that pop up while debugging, we check if we should add them as a debuggee with this function:

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#754

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

Is it was correct reopen this bug? The previous error was fixed, now is going in timeout for a totally different reason – on the same test, true, but `debuggee.runDebuggerStatement` now is a function.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #12)
> Is it was correct reopen this bug? The previous error was fixed, now is
> going in timeout for a totally different reason – on the same test, true,
> but `debuggee.runDebuggerStatement` now is a function.

Split this new stuff off into bug 1049215. Re-resolving this bug.

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> In that case, for finding all debuggee globals, we use the parent's
> |windows| getter:
> 
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> webbrowser.js#551
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> webbrowser.js#635
> 
> And for new globals that pop up while debugging, we check if we should add
> them as a debuggee with this function:
> 
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> webbrowser.js#754

So the change to `onNewGlobal` in https://hg.mozilla.org/mozilla-central/diff/fea2a90e2d22/toolkit/devtools/server/actors/script.js is what caused this test to break.

Basically the new `onNewGlobal` is ignoring page-mods now it seems to me.

Any ideas how we can resolve this Nick?

Can we keep the bit of code that was added in https://github.com/mozilla/gecko-dev/commit/34c7599e4ea29439e8ba6b3f95bd2b3ab72f9f2d#diff-58d9dfd9ca670d23e714c743a045d90eR661 ?

Comment 15

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

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #14)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> > In that case, for finding all debuggee globals, we use the parent's
> > |windows| getter:
> > 
> > http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> > webbrowser.js#551
> > http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> > webbrowser.js#635
> > 
> > And for new globals that pop up while debugging, we check if we should add
> > them as a debuggee with this function:
> > 
> > http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> > webbrowser.js#754
> 
> So the change to `onNewGlobal` in
> https://hg.mozilla.org/mozilla-central/diff/fea2a90e2d22/toolkit/devtools/
> server/actors/script.js is what caused this test to break.
> 
> Basically the new `onNewGlobal` is ignoring page-mods now it seems to me.
> 
> Any ideas how we can resolve this Nick?
> 
> Can we keep the bit of code that was added in
> https://github.com/mozilla/gecko-dev/commit/
> 34c7599e4ea29439e8ba6b3f95bd2b3ab72f9f2d#diff-
> 58d9dfd9ca670d23e714c743a045d90eR661 ?

We still have that code, but its in the tab actor now: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#754

Debuggees are now managed by the parent actor (tab/root/addon), not the thread actor directly.

Comment 16

[Tomislav Jovanovic :zombie]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1574

disabling tests temporarily, a=Mossop

Comment 17

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/c2606b723f2d895efb741403053d5c87c7fdcb05
bug 1042976 - temporarily disable pagemod debugger tests

https://github.com/mozilla/addon-sdk/commit/7db4aade3d69a2ee87c9ee2c001812aca32e1759
Merge pull request #1574 from zombie/1042976-debugger-tests

bug 1042976 - temporarily disable pagemod debugger tests, a=@Mossop

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #15)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #14)
> > (In reply to Nick Fitzgerald [:fitzgen] from comment #11)
> > > In that case, for finding all debuggee globals, we use the parent's
> > > |windows| getter:
> > > 
> > > http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> > > webbrowser.js#551
> > > http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> > > webbrowser.js#635
> > > 
> > > And for new globals that pop up while debugging, we check if we should add
> > > them as a debuggee with this function:
> > > 
> > > http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> > > webbrowser.js#754
> > 
> > So the change to `onNewGlobal` in
> > https://hg.mozilla.org/mozilla-central/diff/fea2a90e2d22/toolkit/devtools/
> > server/actors/script.js is what caused this test to break.
> > 
> > Basically the new `onNewGlobal` is ignoring page-mods now it seems to me.
> > 
> > Any ideas how we can resolve this Nick?
> > 
> > Can we keep the bit of code that was added in
> > https://github.com/mozilla/gecko-dev/commit/
> > 34c7599e4ea29439e8ba6b3f95bd2b3ab72f9f2d#diff-
> > 58d9dfd9ca670d23e714c743a045d90eR661 ?
> 
> We still have that code, but its in the tab actor now:
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> webbrowser.js#754
> 
> Debuggees are now managed by the parent actor (tab/root/addon), not the
> thread actor directly.

Ah so there is a `getInnerId in undefined` error that is being silenced.

Comment 19

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

But it is the same that it used to be: http://hg.mozilla.org/mozilla-central/rev/fea2a90e2d22#l5.288

Comment 20

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #19)
> But it is the same that it used to be:
> http://hg.mozilla.org/mozilla-central/rev/fea2a90e2d22#l5.288

So the call to `getInnerId(...)` got moved from a file that had a getInnerId function defined to a file that does not have it defined.

From http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js
To http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js

So the `getInnerId` function in the former file should be moved to the latter file I suppose.

Comment 21

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

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #20)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #19)
> > But it is the same that it used to be:
> > http://hg.mozilla.org/mozilla-central/rev/fea2a90e2d22#l5.288
> 
> So the call to `getInnerId(...)` got moved from a file that had a getInnerId
> function defined to a file that does not have it defined.
> 
> From
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> script.js
> To
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> webbrowser.js
> 
> So the `getInnerId` function in the former file should be moved to the
> latter file I suppose.

D'oh! Good catch. Will have a patch up shortly.

Comment 22

[Dave Townsend [:mossop]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #21)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #20)
> > (In reply to Nick Fitzgerald [:fitzgen] from comment #19)
> > > But it is the same that it used to be:
> > > http://hg.mozilla.org/mozilla-central/rev/fea2a90e2d22#l5.288
> > 
> > So the call to `getInnerId(...)` got moved from a file that had a getInnerId
> > function defined to a file that does not have it defined.
> > 
> > From
> > http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> > script.js
> > To
> > http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> > webbrowser.js
> > 
> > So the `getInnerId` function in the former file should be moved to the
> > latter file I suppose.
> 
> D'oh! Good catch. Will have a patch up shortly.

Did that ever happen?

Comment 23

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

Attachment: follow up - Remove getInnerId from script actors

Comment 24

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

(In reply to Dave Townsend [:mossop] from comment #22)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #21)
> > (In reply to Erik Vold [:erikvold] [:ztatic] from comment #20)
> > > (In reply to Nick Fitzgerald [:fitzgen] from comment #19)
> > > > But it is the same that it used to be:
> > > > http://hg.mozilla.org/mozilla-central/rev/fea2a90e2d22#l5.288
> > > 
> > > So the call to `getInnerId(...)` got moved from a file that had a getInnerId
> > > function defined to a file that does not have it defined.
> > > 
> > > From
> > > http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> > > script.js
> > > To
> > > http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> > > webbrowser.js
> > > 
> > > So the `getInnerId` function in the former file should be moved to the
> > > latter file I suppose.
> > 
> > D'oh! Good catch. Will have a patch up shortly.
> 
> Did that ever happen?

Yeah, but looks like we have a leftover copy of the function in the old file, hence the followup patch.

Comment 25

[Dave Townsend [:mossop]]

Well the test is still failing for some reason, filed bug 1143136

Comment 26

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

https://hg.mozilla.org/integration/fx-team/rev/d4e6c0e3be81

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d4e6c0e3be81