Closed Bug 1300036 Opened 8 years ago Closed 8 years ago

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

Categories

(DevTools :: Debugger, defect, P3)

Product:
DevTools
Component:
Debugger
Type:
defect

Tracking

(firefox50 unaffected, firefox51 fixed, firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- fixed
firefox52 --- fixed

People

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

References

Details

(Keywords: intermittent-failure)

Attachments

(3 files)

Bug 1300036 - Ensure waiting for source shown when reloading tab in debugger tests.
58 bytes, text/x-review-board-request
ejpbruel
: review+
Details
Bug 1300036 - Update the state correctly on the client side when the server reports a conflict on resume.
58 bytes, text/x-review-board-request
ejpbruel
: review+
ritu
: approval-mozilla-aurora+
Details
reenable the test
1.04 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review 
Starting to wonder if it's maybe not a coincidence that this started on https://hg.mozilla.org/integration/autoland/rev/42ada9dc0ab3c9e872f56201a0a1c4f7eb685acc
I'm more suspicious of bug 1243452.
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
Oh. And the test itself is broken, it doesn't wait correctly when reloading the tab.
Two patches coming.
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.
Priority: -- → P3
James, Should I just disable this test or look for another reviewer?
Flags: needinfo?(jlong)
Here, let me get that for you.
Keywords: leave-open
Whiteboard: [test disabled]
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
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 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 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+
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.
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.
I requested mozreview to land the test fixes and will try to reenable the test tomorrow.
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
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)
Attachment #8791557 - Flags: review?(ejpbruel) → review+
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
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/9e79393e2821
Status: NEW → RESOLVED
Closed: 8 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]
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+
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
(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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: