Intermittent browser_toolbox_view_source_01.js | The correct line is highlighted in the debugger's source editor. - Got 1, expected 2

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Developer Tools
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: philor, Assigned: Sami Jaktholm)

Tracking

({intermittent-failure})

Trunk
Firefox 49
intermittent-failure
Points:
---

Firefox Tracking Flags

(firefox45 affected, firefox49 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

Comment 1

2 years ago
6 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 3
* fx-team: 2
* try: 1

Platform breakdown:
* windows7-32: 6

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

Comment 2

2 years ago
7 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 6
* fx-team: 1

Platform breakdown:
* windows7-32: 6
* osx-10-6: 1

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

Comment 3

2 years ago
11 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 7
* fx-team: 3
* try: 1

Platform breakdown:
* linux64: 8
* linux32: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1230990&startday=2016-05-16&endday=2016-05-22&tree=all
(Assignee)

Comment 4

2 years ago
Created attachment 8757121 [details]
MozReview Request: Bug 1230990 - Wait for the source text to finish loading before signaling that source has been shown in the debugger. r?ochameau

Currently viewSourceInDebugger() only waits for a source to be shown if it is
not the selected one. If the requested source is selected by default, it might
already be selected when the isLoading check is made. However, the actual
source text might still be loading since it is loaded asynchronously.

In this case the method does not wait for the source to be shown meaning the
method resolves before the source is ready in the debugger. This causes
browser_toolbox_view_source_01.js to fail intermittently with wrong
line number since it looks at the focused line BEFORE the source text has
loaded and the correct line is selected.

This changeset modifies the behavior of viewSourceInDebugger() to also wait for
source shown event if the source is already selected but still loading.

Review commit: https://reviewboard.mozilla.org/r/55664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55664/
Attachment #8757121 - Flags: review?(jryans)
(Assignee)

Comment 5

2 years ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a97e950f320
Assignee: nobody → sjakthol
Status: NEW → ASSIGNED
Comment on attachment 8757121 [details]
MozReview Request: Bug 1230990 - Wait for the source text to finish loading before signaling that source has been shown in the debugger. r?ochameau

:ochameau has been working on Debugger tests recently, I think he is a better reviewer for this.
Attachment #8757121 - Flags: review?(jryans) → review?(poirot.alex)

Comment 7

2 years ago
10 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 3
* fx-team: 3
* try: 1
* mozilla-central: 1
* mozilla-aurora: 1
* ash: 1

Platform breakdown:
* linux64: 5
* osx-10-10: 3
* osx-10-6: 1
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1230990&startday=2016-05-23&endday=2016-05-29&tree=all
Comment on attachment 8757121 [details]
MozReview Request: Bug 1230990 - Wait for the source text to finish loading before signaling that source has been shown in the debugger. r?ochameau

https://reviewboard.mozilla.org/r/55664/#review52854

Makes sense, thanks for figuring this out!

I just have a small code tweak to suggest...

::: devtools/client/shared/view-source.js:86
(Diff revision 1)
> +    const selected = state.sources.selectedSource;
> +    const isSelected = selected === actor;
> +
> +    // (2) Has the source text finished loading?
> +    const sourceTextInfo = getSourceText(state, selected);
> +    const isLoading = sourceTextInfo && sourceTextInfo.loading;

Shouldn't you only compute isLoading and sourceTextInfo if isSelected is true?
Otherwise you are going to compute useless things for another source.
Attachment #8757121 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 9

2 years ago
Comment on attachment 8757121 [details]
MozReview Request: Bug 1230990 - Wait for the source text to finish loading before signaling that source has been shown in the debugger. r?ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55664/diff/1-2/
Attachment #8757121 - Attachment description: MozReview Request: Bug 1230990 - Wait for the source text to finish loading before signaling that source has been shown in the debugger. r?jryans → MozReview Request: Bug 1230990 - Wait for the source text to finish loading before signaling that source has been shown in the debugger. r?ochameau
(Assignee)

Comment 10

2 years ago
Wrapped the second check to an if-clause. Let's see how that went: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3b44d9bb79f
(Assignee)

Comment 11

2 years ago
Hmm... Bugzilla says r+, reviewboard r?. Could you take a look at the changes?
Flags: needinfo?(poirot.alex)
Flags: needinfo?(poirot.alex)
Looks good.

FYI, see bug 1276073 where I apply similar fix to all debugger tests.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 14

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/b30ef0b6848b
Wait for the source text to finish loading before signaling that source has been shown in the debugger. r=ochameau
Keywords: checkin-needed

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b30ef0b6848b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.