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. -

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Developer Tools: Debugger
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Treeherder Bug Filer, Assigned: ochameau)

Tracking

({intermittent-failure})

unspecified
Firefox 52
intermittent-failure
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 unaffected, firefox51 fixed, firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

a year ago
treeherder
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
Starting to wonder if it's maybe not a coincidence that this started on https://hg.mozilla.org/integration/autoland/rev/42ada9dc0ab3c9e872f56201a0a1c4f7eb685acc

Comment 2

a year ago
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

a year ago
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
(Assignee)

Comment 4

a year ago
I'm more suspicious of bug 1243452.
(Assignee)

Comment 5

a year ago
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".
Assignee: nobody → poirot.alex
Blocks: 1299602
(Assignee)

Comment 6

a year ago
Oh. And the test itself is broken, it doesn't wait correctly when reloading the tab.
Two patches coming.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

a year ago
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
(Assignee)

Comment 10

a year ago
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 hidden (mozreview-request)
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
Priority: -- → P3
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
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
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
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
(Assignee)

Comment 17

a year ago
James, Should I just disable this test or look for another reviewer?
Flags: needinfo?(jlong)
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
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
Here, let me get that for you.
Keywords: leave-open
Whiteboard: [test disabled]

Comment 21

a year ago
Pushed by philringnalda@gmail.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/24a5b1c8243e
(Assignee)

Comment 23

a year ago
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.
Flags: needinfo?(ejpbruel)

Comment 24

a year ago
mozreview-review
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.
Attachment #8788232 - Flags: review+
Attachment #8788231 - Flags: review?(jlong) → review?(ejpbruel)
Attachment #8788232 - Flags: review?(jlong)

Comment 25

a year ago
mozreview-review
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.
Attachment #8788231 - Flags: review?(ejpbruel) → review+
(Assignee)

Comment 26

a year ago
mozreview-review-reply
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.
(Assignee)

Updated

a year ago
Flags: needinfo?(jlong)
Flags: needinfo?(ejpbruel)
(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.
(Assignee)

Comment 28

a year ago
I requested mozreview to land the test fixes and will try to reenable the test tomorrow.
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

a year ago
Pushed by apoirot@mozilla.com:
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
(Assignee)

Comment 31

a year ago
Created attachment 8791557 [details] [diff] [review]
reenable the test
(Assignee)

Comment 32

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6554041cbac
(Assignee)

Comment 33

a year ago
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
Attachment #8791557 - Flags: review?(ejpbruel)

Comment 34

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d9a19f976f1e
https://hg.mozilla.org/mozilla-central/rev/73f77b885f17
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
Attachment #8791557 - Flags: review?(ejpbruel) → review+
(Assignee)

Comment 36

a year ago
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
(Assignee)

Updated

a year ago
Keywords: leave-open

Comment 37

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9e79393e2821
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Please request approval for the client-side patch when you get a chance (the rest is test-only and doesn't need approval).
status-firefox50: --- → unaffected
status-firefox51: --- → affected
Flags: needinfo?(poirot.alex)
Whiteboard: [test disabled]
(Assignee)

Comment 39

a year ago
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
Flags: needinfo?(poirot.alex)
Attachment #8788232 - Flags: approval-mozilla-beta?
Attachment #8788232 - Flags: approval-mozilla-aurora?
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+
Attachment #8788232 - Flags: approval-mozilla-beta?
Attachment #8788232 - Flags: approval-mozilla-beta+
Attachment #8788232 - Flags: approval-mozilla-aurora?
Attachment #8788232 - Flags: approval-mozilla-aurora+

Updated

a year ago
status-firefox50: unaffected → affected
https://hg.mozilla.org/releases/mozilla-aurora/rev/b3d665af3124
status-firefox51: affected → fixed
Flags: in-testsuite+
https://hg.mozilla.org/releases/mozilla-beta/rev/620464bdbdff
https://hg.mozilla.org/releases/mozilla-beta/rev/84899036528a
status-firefox50: affected → fixed
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
status-firefox50: fixed → unaffected
(Assignee)

Comment 44

a year ago
(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 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
Attachment #8788232 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.