Sources are blank if Auto Prettify Minified Sources enabled

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Developer Tools: Debugger
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: arni2033, Assigned: jlongster)

Tracking

({regression})

Trunk
Firefox 48
regression
Points:
---

Firefox Tracking Flags

(firefox45 wontfix, firefox46+ wontfix, firefox47+ fixed, firefox48+ fixed, firefox49 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
>>>   My Info:   Win7_64, Nightly 48, 32bit, ID 20160326030430
STR_1:
1. Open attachment 8724506 [details]
2. Open Debugger, enable "Auto Prettify Minified Sources"
3. Close and reopen devtools

AR:  No code displayed in the code area.
ER:  The code should be displayed.


Notes:
1) This bug is different from "sources are missing". I filed that as bug 1211514, no need to report it
2) In this bug, every time a script is displayed in the list, the code area is empty. That is the bug
3) If you want to workaround bug 1211514, then in Step 3 you should:
     Switch to Inspector tab -> close and reopen devtools -> Click (ev) button near <html> ->
     -> click pause button in event listener popup to switch to Debugger.
   That would reduce the frequency of bug 1211514.


STR_2:
1. Open http://www.vesti.ru/doc.html?id=2735707 , wait until the site is fully loaded.
2. Open Debugger, enable "Auto Prettify Minified Sources"
3. Close and reopen devtools
4. If the script selected in the right sidebar is not "26sdov5axQhN", switch to that script


This bug is regression from bug 1200798. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=15522bc2931a52ec435f22e233f747264b90f647&tochange=0e47cb06470145725732f0f362c46048969126d8
(Reporter)

Comment 1

2 years ago
Correction:   there's a typo in STR_2, Step 4:
4. If the script selected in the LEFT sidebar is not "26sdov5axQhN", switch to that script
James, it seems like this is another regression with auto-prettify.  Can you take a look?

You did recently fix a related problem in bug 1243561, but this variant still exists in Nightly 48.
Flags: needinfo?(jlong)
(Assignee)

Comment 3

2 years ago
Created attachment 8735449 [details] [diff] [review]
1260042.patch

Thanks for filing this. Unrelated to the other bug. This happens because prettifying the text fails (because it's not JS, it's HTML) but the editor gets confused because it think it's already displaying the source so it doesn't load in the real source when it comes in. The editor has internal state and only updates the text if it thinks something has changed, and this internal state is wrong.

A small fix that I'm going to quickly land.
Flags: needinfo?(jlong)
(Assignee)

Comment 4

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb72efbe86a1
(Assignee)

Updated

2 years ago
Assignee: nobody → jlong
(Assignee)

Updated

2 years ago
Priority: -- → P1
[Tracking Requested - why for this release]: Regression reproduces back to beta.

From the regression window, it may be in release as well, be normally we ignore that for DevTools bugs.
status-firefox45: --- → wontfix
status-firefox46: --- → affected
status-firefox47: --- → affected
tracking-firefox46: --- → ?
tracking-firefox47: --- → ?

Comment 6

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/6735565943d3

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6735565943d3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
jryans, want to uplift this? We could still take it in beta 7 later this week.
tracking-firefox46: ? → +
tracking-firefox47: ? → +
tracking-firefox48: --- → +
Flags: needinfo?(jryans)
Redirecting to uplift questions assignee.
Flags: needinfo?(jryans) → needinfo?(jlong)
That should have been "Redirect uplift questions to assignee." :)
(Assignee)

Comment 11

2 years ago
I can uplift, but I haven't added any tests because I wanted to land P1 fixes quickly and we're working on writing new tests. I wasn't planning on uplifting, but it is a small change I guess.
Flags: needinfo?(jlong)
(Assignee)

Comment 12

2 years ago
Comment on attachment 8735449 [details] [diff] [review]
1260042.patch

Approval Request Comment
[Feature/regressing bug #]: bug 1200798
[User impact if declined]: If the source fails to pretty-print on startup, the source won't be shown in the debugger. 
[Describe test coverage new/current, TreeHerder]: No new test coverage as we are working on a new test suite and this is a 2 line fix.
[Risks and why]: No risk. Only affects debugger and it's a 2 line change.
[String/UUID change made/needed]:
Attachment #8735449 - Flags: approval-mozilla-aurora?

Comment 13

2 years ago
(In reply to James Long (:jlongster) from comment #11)
> I can uplift, but I haven't added any tests because I wanted to land P1
> fixes quickly and we're working on writing new tests. I wasn't planning on
> uplifting, but it is a small change I guess.

So, tests are one way we control risk. I think we've settled on some principles about how we control risk in Devtools, and we should deviate from them deliberately. This case seems kind of impulsive. 

The textual size of the change is irrelevant, of course; what matters is its impact on behavior. The patch does not look entirely trivial to me: we've changed the conditions under which an event is emitted, haven't we?
(Assignee)

Comment 14

2 years ago
Comment on attachment 8735449 [details] [diff] [review]
1260042.patch

Cancelling uplift as per IRC. Should always have a test if uplifting, I'll add one tomorrow.
Attachment #8735449 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 15

2 years ago
When I perform STR_2 on current Nightly (ID 20160330030326), script "26sdov5axQhN" looks minified.
So according to the pref "Auto Prettify Minified Sources", it should be pretty printed, but it's not
Is that according to the plan?
(Reporter)

Updated

2 years ago
Flags: needinfo?(jlong)
(Assignee)

Comment 16

2 years ago
(In reply to arni2033 from comment #15)
> When I perform STR_2 on current Nightly (ID 20160330030326), script
> "26sdov5axQhN" looks minified.
> So according to the pref "Auto Prettify Minified Sources", it should be
> pretty printed, but it's not
> Is that according to the plan?

Yes. It's an HTML file, and we only pretty-print JS files. We don't know how to pretty-print mixed HTML/JS/CSS files.
Flags: needinfo?(jlong)
(Reporter)

Comment 17

2 years ago
(In reply to James Long (:jlongster) from comment #16)
> Yes. It's an HTML file, and we only pretty-print JS files. We don't know how
> to pretty-print mixed HTML/JS/CSS files.
1) That file is pure JS. In comment 0 I just listed all regressed sources I could find.
2) I saw some mixed files that can be pretty-printed even in Firefox Debugger (e.g. in bug 1258830)
3) (sorry, I literally _just_ checked) It's regression from bug 1200798. I filed bug 1261134.

I can verify this particular bug, which is "Sources are blank".
Fixed on:   Win7_64, Nightly 48, 32bit, ID 20160330030326
Status: RESOLVED → VERIFIED
Wontfix for 46.
status-firefox46: affected → wontfix
Hi James, should we consider uplifting this to Beta47? This was marked as P1 regression and therefore caught my attention.
Flags: needinfo?(jlong)
Hi Jen, Joe, could you please help determine if this needs to be uplifted to Fx47? If yes, please nominate for uplift as requested by me in comment 19. Thanks!
Flags: needinfo?(jwalker)
Flags: needinfo?(jfong)
(Assignee)

Comment 21

2 years ago
I'm sorry I've been slow on this -- I've been focusing on a different project for a little while now. I need to write a test for this and then I think we should uplift this. I will do that tomorrow.
Flags: needinfo?(jlong)
See comment 21
Flags: needinfo?(jwalker)
(Assignee)

Comment 23

2 years ago
Re-opening to land a test for this. I will generate a patch with the test to be uplifted to beta.
Status: VERIFIED → REOPENED
Flags: needinfo?(jfong)
Resolution: FIXED → ---
(Assignee)

Comment 24

2 years ago
Created attachment 8750896 [details] [diff] [review]
1260042-test.patch
(Assignee)

Comment 25

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4be5d9f5825a

Comment 26

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/a67f6710c37c

Comment 27

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a67f6710c37c
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
(Assignee)

Comment 28

2 years ago
Created attachment 8752991 [details] [diff] [review]
1260042-uplift.patch
(Assignee)

Comment 29

2 years ago
Comment on attachment 8752991 [details] [diff] [review]
1260042-uplift.patch

Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: Auto-pretty-printing may not work in debugger
[Describe test coverage new/current, TreeHerder]: New test added, has been on nightly for a while
[Risks and why]: Very little risk, only a few lines changed and a new test has been added
[String/UUID change made/needed]:
Attachment #8752991 - Flags: approval-mozilla-beta?
Attachment #8752991 - Flags: approval-mozilla-aurora?
Comment on attachment 8752991 [details] [diff] [review]
1260042-uplift.patch

Fix was verified on Aurora48, Nightly49. Not sure if this needs uplift to 48. Beta47+
Attachment #8752991 - Flags: approval-mozilla-beta?
Attachment #8752991 - Flags: approval-mozilla-beta+
Attachment #8752991 - Flags: approval-mozilla-aurora?
Attachment #8752991 - Flags: approval-mozilla-aurora+
I'm hitting conflicts uplifting this to aurora:

hg import -e https://bugzilla.mozilla.org/attachment.cgi?id=8752991
applying https://bugzilla.mozilla.org/attachment.cgi?id=8752991
patching file devtools/client/debugger/content/reducers/sources.js
Hunk #1 FAILED at 61
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/debugger/content/reducers/sources.js.rej
patching file devtools/client/shared/redux/non-react-subscriber.js
Hunk #1 FAILED at 83
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/shared/redux/non-react-subscriber.js.rej
abort: patch failed to apply
Flags: needinfo?(jlong)
(Assignee)

Comment 32

a year ago
(In reply to Wes Kocher (:KWierso) from comment #31)
> I'm hitting conflicts uplifting this to aurora:
> 
> hg import -e https://bugzilla.mozilla.org/attachment.cgi?id=8752991
> applying https://bugzilla.mozilla.org/attachment.cgi?id=8752991
> patching file devtools/client/debugger/content/reducers/sources.js
> Hunk #1 FAILED at 61
> 1 out of 1 hunks FAILED -- saving rejects to file
> devtools/client/debugger/content/reducers/sources.js.rej
> patching file devtools/client/shared/redux/non-react-subscriber.js
> Hunk #1 FAILED at 83
> 1 out of 1 hunks FAILED -- saving rejects to file
> devtools/client/shared/redux/non-react-subscriber.js.rej
> abort: patch failed to apply

I didn't realize this, but it's actually already been uplifted to aurora. This needs to be uplifted to beta only.
Flags: needinfo?(jlong)

Comment 33

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/28a1db8e260e
status-firefox47: affected → fixed
You need to log in before you can comment on or make changes to this bug.