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

P3
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: intermittent-bug-filer, Assigned: ochameau)

Tracking

({intermittent-failure})

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

Firefox Tracking Flags

(firefox50 unaffected, firefox51 fixed, firefox52 fixed)

Details

Attachments

(3 attachments)

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

2 years ago
I'm more suspicious of bug 1243452.
(Assignee)

Comment 5

2 years 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

2 years 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)
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

2 years 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

Updated

2 years ago
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

2 years 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

2 years 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
(Assignee)

Comment 23

2 years 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

2 years 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+

Updated

2 years ago
Attachment #8788231 - Flags: review?(jlong) → review?(ejpbruel)
Attachment #8788232 - Flags: review?(jlong)

Comment 25

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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

2 years ago
Created attachment 8791557 [details] [diff] [review]
reenable the test
(Assignee)

Comment 33

2 years 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)
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

Updated

2 years ago
Attachment #8791557 - Flags: review?(ejpbruel) → review+
(Assignee)

Comment 36

2 years 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

2 years ago
Keywords: leave-open

Comment 37

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9e79393e2821
Status: NEW → RESOLVED
Last Resolved: 2 years 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

2 years 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

2 years ago
status-firefox50: unaffected → affected

Comment 41

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/b3d665af3124
status-firefox51: affected → fixed
Flags: in-testsuite+
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

2 years 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+

Updated

5 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.