1300036 - Intermittent devtools/client/debugger/test/mochitest/browser_dbg_pretty-print-11.js | You should use 'resumeDebuggerThenCloseAndFinish' instead, unless you're absolutely sure about what you're doing. -

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Treeherder Bug Filer]

Filed by: philringnalda [at] gmail.com

https://treeherder.mozilla.org/logviewer.html#?job_id=2949167&repo=autoland

https://queue.taskcluster.net/v1/task/W_CfELZoTR2VpLhxWhEmrw/runs/0/artifacts/public%2Flogs%2Flive_backing.log

Comment 1

[Phil Ringnalda (:philor)]

Starting to wonder if it's maybe not a coincidence that this started on https://hg.mozilla.org/integration/autoland/rev/42ada9dc0ab3c9e872f56201a0a1c4f7eb685acc

Comment 2

[Intermittent Failures Robot]

16 automation job failures were associated with this bug yesterday.

Repository breakdown:
* autoland: 7
* mozilla-inbound: 6
* mozilla-central: 2
* fx-team: 1

Platform breakdown:
* linux64: 14
* windows7-32-vm: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1300036&startday=2016-09-03&endday=2016-09-03&tree=all

Comment 3

[Intermittent Failures Robot]

40 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* autoland: 21
* mozilla-inbound: 10
* mozilla-central: 6
* fx-team: 3

Platform breakdown:
* linux64: 29
* osx-10-10: 5
* windows7-32-vm: 4
* linux32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1300036&startday=2016-08-29&endday=2016-09-04&tree=all

Comment 4

[Alexandre Poirot [:ochameau]]

I'm more suspicious of bug 1243452.

Comment 5

[Alexandre Poirot [:ochameau]]

No, you are right, that's because of bug 1299602!
ThreadClient.resume is kind of broken...
It tries to reset the state to "paused" if any error happened during the request:
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/client/main.js#1756-1759
But it happens that bug 1299602 kicks in in middle of a resume() request.
bug 1299602's patch changes the state to "attached", but this code from ThreadClient.resume reset the state back to "paused".

Comment 6

[Alexandre Poirot [:ochameau]]

Oh. And the test itself is broken, it doesn't wait correctly when reloading the tab.
Two patches coming.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1300036 - Ensure waiting for source shown when reloading tab in debugger tests.

Review commit: https://reviewboard.mozilla.org/r/76796/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76796/

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1300036 - Update the state correctly on the client side when the server reports a conflict on resume.

Review commit: https://reviewboard.mozilla.org/r/76798/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76798/

Comment 9

[Intermittent Failures Robot]

18 automation job failures were associated with this bug yesterday.

Repository breakdown:
* autoland: 7
* mozilla-inbound: 6
* mozilla-central: 2
* fx-team: 2
* try: 1

Platform breakdown:
* linux64: 16
* windows7-32-vm: 1
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1300036&startday=2016-09-05&endday=2016-09-05&tree=all

Comment 10

[Alexandre Poirot [:ochameau]]

devtools/server/tests/unit/test_nesting-03.js expects the state to be reseted when error is wrongState,
so I have to tweak the second patch.

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8788232 [details]
Bug 1300036 - Update the state correctly on the client side when the server reports a conflict on resume.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76798/diff/1-2/

Comment 12

[Intermittent Failures Robot]

43 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 20
* autoland: 12
* fx-team: 7
* mozilla-central: 4

Platform breakdown:
* linux64: 36
* osx-10-10: 5
* windows7-32-vm: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1300036&startday=2016-09-06&endday=2016-09-06&tree=all

Comment 13

[Intermittent Failures Robot]

53 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 26
* autoland: 19
* fx-team: 3
* try: 2
* mozilla-central: 2
* ash: 1

Platform breakdown:
* linux64: 48
* osx-10-10: 2
* linux32: 2
* windows7-32-vm: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1300036&startday=2016-09-07&endday=2016-09-07&tree=all

Comment 14

[Intermittent Failures Robot]

54 automation job failures were associated with this bug yesterday.

Repository breakdown:
* autoland: 22
* mozilla-inbound: 20
* mozilla-central: 5
* fx-team: 5
* ash: 2

Platform breakdown:
* linux64: 46
* linux32: 4
* windows7-32-vm: 3
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1300036&startday=2016-09-08&endday=2016-09-08&tree=all

Comment 15

[Intermittent Failures Robot]

33 automation job failures were associated with this bug yesterday.

Repository breakdown:
* autoland: 18
* mozilla-inbound: 8
* mozilla-central: 4
* fx-team: 3

Platform breakdown:
* linux64: 27
* linux32: 3
* osx-10-10: 2
* windows7-32-vm: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1300036&startday=2016-09-09&endday=2016-09-09&tree=all

Comment 16

[Intermittent Failures Robot]

236 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 89
* autoland: 82
* fx-team: 33
* mozilla-central: 25
* ash: 4
* try: 3

Platform breakdown:
* linux64: 196
* osx-10-10: 18
* linux32: 13
* windows7-32-vm: 8
* windows8-64: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1300036&startday=2016-09-05&endday=2016-09-11&tree=all

Comment 17

[Alexandre Poirot [:ochameau]]

James, Should I just disable this test or look for another reviewer?

Comment 18

[Intermittent Failures Robot]

41 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 20
* autoland: 11
* fx-team: 8
* mozilla-central: 2

Platform breakdown:
* linux64: 31
* osx-10-10: 6
* windows7-32-vm: 2
* linux32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1300036&startday=2016-09-12&endday=2016-09-12&tree=all

Comment 19

[Intermittent Failures Robot]

32 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-inbound: 12
* mozilla-central: 7
* autoland: 7
* try: 5
* fx-team: 1

Platform breakdown:
* linux64: 27
* osx-10-10: 4
* windows7-32-vm: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1300036&startday=2016-09-13&endday=2016-09-13&tree=all

Comment 20

[Phil Ringnalda (:philor)]

Here, let me get that for you.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24a5b1c8243e
Disable browser_dbg_pretty-print-11.js until it's fixed to work correctly and not constantly fail

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/24a5b1c8243e

Comment 23

[Alexandre Poirot [:ochameau]]

Eddy, we are about to burry broken things behind the old debugger. Do we still maintain old debugger tests? Should we agressively disable them?
This test was highlighting things being racy in ThreadClient.resume, it will impact the new debugger if it still uses ThreadClient.

Comment 24

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8788232 [details]
Bug 1300036 - Update the state correctly on the client side when the server reports a conflict on resume.

https://reviewboard.mozilla.org/r/76798/#review77292

This seems like a clean fix to the problem. Well done Alex.

Comment 25

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8788231 [details]
Bug 1300036 - Ensure waiting for source shown when reloading tab in debugger tests.

https://reviewboard.mozilla.org/r/76796/#review77294

LGTM

::: devtools/client/debugger/test/mochitest/browser_dbg_breakpoints-reload.js:30
(Diff revision 1)
>      actor: sources.selectedValue,
>      line: 10 // "break on me" string
>    });
>  
>    const paused = waitForThreadEvents(panel, "paused");
> -  reloadActiveTab(panel);
> +  yield reloadActiveTab(panel, panel.panelWin.EVENTS.SOURCE_SHOWN);

Unlike the other instances, this particular change seems unnecessary? We don't test any UI state between lines 30-38, so we can afford to wait for the sources to be shown until then.

Comment 26

[Alexandre Poirot [:ochameau]]

Comment on attachment 8788231 [details]
Bug 1300036 - Ensure waiting for source shown when reloading tab in debugger tests.

https://reviewboard.mozilla.org/r/76796/#review77294

> Unlike the other instances, this particular change seems unnecessary? We don't test any UI state between lines 30-38, so we can afford to wait for the sources to be shown until then.

Even if we don't assert any UI state, it is still better to wait for any async step to prevent random exception when we are going to close the toolbox and shutdown the client on test end.

Comment 27

[James Long (:jlongster)]

(In reply to Alexandre Poirot [:ochameau] from comment #17)
> James, Should I just disable this test or look for another reviewer?

I'm very sorry, I was so focused on launching the new one earlier this week.

We don't explicitly maintain the old tests, but are looking through them and porting the important ones. I would say still fix them when you can. We are probably a couple months away from actually removing them.

Thank you Eddy for looking at this.

Comment 28

[Alexandre Poirot [:ochameau]]

I requested mozreview to land the test fixes and will try to reenable the test tomorrow.

Comment 29

[Intermittent Failures Robot]

26 automation job failures were associated with this bug yesterday.

Repository breakdown:
* autoland: 17
* mozilla-inbound: 4
* fx-team: 4
* mozilla-central: 1

Platform breakdown:
* linux64: 24
* osx-10-10: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1300036&startday=2016-09-14&endday=2016-09-14&tree=all

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9a19f976f1e
Ensure waiting for source shown when reloading tab in debugger tests. r=ejpbruel
https://hg.mozilla.org/integration/autoland/rev/73f77b885f17
Update the state correctly on the client side when the server reports a conflict on resume. r=ejpbruel

Comment 31

[Alexandre Poirot [:ochameau]]

Attachment: reenable the test

Comment 32

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6554041cbac

Comment 33

[Alexandre Poirot [:ochameau]]

Comment on attachment 8791557 [details] [diff] [review]
reenable the test

Review of attachment 8791557 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like we should be able to reenable it:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6554041cbac

Comment 34

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

https://hg.mozilla.org/mozilla-central/rev/d9a19f976f1e
https://hg.mozilla.org/mozilla-central/rev/73f77b885f17

Comment 35

[Intermittent Failures Robot]

100 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 37
* autoland: 35
* fx-team: 13
* mozilla-central: 10
* try: 5

Platform breakdown:
* linux64: 83
* osx-10-10: 11
* windows7-32-vm: 3
* linux32: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1300036&startday=2016-09-12&endday=2016-09-18&tree=all

Comment 36

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/9e79393e28212aebf59204d6b39921aaf3e0804a
Bug 1300036 - Reenable browser_dbg_pretty-print-11.js now that it is no longer intermittent. r=ejpbruel

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e79393e2821

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Please request approval for the client-side patch when you get a chance (the rest is test-only and doesn't need approval).

Comment 39

[Alexandre Poirot [:ochameau]]

Comment on attachment 8788232 [details]
Bug 1300036 - Update the state correctly on the client side when the server reports a conflict on resume.

Approval Request Comment
[Feature/regressing bug #]: always existed
[User impact if declined]: Might happen while stressing the debugger or on very slow hardware. But haven't heard about any report. The debugger might break itself when reloading website while a breakpoint has been hit.
[Describe test coverage new/current, TreeHerder]: test were failing intermittently because of this issue. No specific test were introduced as it is related to a very hard to reproduce timing issue.
[Risks and why]: Mostly to get rid of intermittent on other branches. But very narrowed patch against one request made by the debugger ui.
[String/UUID change made/needed]: no

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8788232 [details]
Bug 1300036 - Update the state correctly on the client side when the server reports a conflict on resume.

Fixes a bug (caught by an intermittent test failure) in the debugger on slow machines, seems low risk, Aurora51+, Beta50+

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b3d665af3124

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/620464bdbdff
https://hg.mozilla.org/releases/mozilla-beta/rev/84899036528a

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Backed out for making the test extremely failure-prone on Beta. I had marked 50 as unaffected back in comment 38, so I'm not convinced this needs uplifting there unless there's an underlying devtools issue we really want to fix? If so, there'll need to be some investigation into why it's failing there.

https://treeherder.mozilla.org/logviewer.html#?job_id=1681814&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/ab97d0cfa0a3

Comment 44

[Alexandre Poirot [:ochameau]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #43)
> Backed out for making the test extremely failure-prone on Beta. I had marked
> 50 as unaffected back in comment 38, so I'm not convinced this needs
> uplifting there unless there's an underlying devtools issue we really want
> to fix? If so, there'll need to be some investigation into why it's failing
> there.

The fix isn't critical, it is much more usefull to just have green tests.
So it isn't worth looking at why it fails on beta, it is most likely missing another intermittent fix...
(I thought it was missing bug 1276073 or bug 1080025, but they both landed for 49)

Comment 45

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8788232 [details]
Bug 1300036 - Update the state correctly on the client side when the server reports a conflict on resume.

Clearing the flag as we decided to back this out of Beta50