1128747 - Style Editor: Extra white space appearing above the editor for a sourcemapped scss file

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Attachment: gulp-sourcemaps-bug.png

WIll update with repro url in a sec

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

repro URL http://canuckistani.github.io/bug-1128747/

Comment 2

[Brian Grinstead [:bgrins]]

Confirmed - I'm seeing it on 38, but not 37

Comment 3

[Brian Grinstead [:bgrins]]

Actually, I'm seeing it in 37 too

Comment 4

[Brian Grinstead [:bgrins]]

There is an error right away when loading Style Editor panel (when it would usually be opening up an editor with the first source):

TypeError: env.contentWindow is undefined editor.js:256:10

Then when you click on the sheet on the left it creates a new editor, so there are actually two editor iframes here.

Relevant line: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/editor.js#256

Comment 5

[Brian Grinstead [:bgrins]]

The onLoad event seems to be firing at the wrong time (and there is not even a contentWindow defined on the iframe).   It's not clear why this is different using original sources here, I'll have to look into it further.

Comment 6

[Jeff Griffiths (:canuckistani) (:⚡︎)]

[Tracking Requested - why for this release]:

We can reproduce this issue on beta, dev edition and nightly.

Brian: when you figure out a fix, we'll need to uplift as far as Beta, I can repro there as well but it seems a bit more intermittent.

Comment 7

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Also, to be clear, release channel is not effected.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Sounds like this is specific to Dev Edition in that case, and doesn't carry on to Beta/Release which means it's not a tracking issue (for release issues) but an uplift needed to mozilla-aurora branch can be considered when ready.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #8)
> Sounds like this is specific to Dev Edition in that case, and doesn't carry
> on to Beta/Release which means it's not a tracking issue (for release
> issues) but an uplift needed to mozilla-aurora branch can be considered when
> ready.

I don't think it is specific to Dev Edition - we are seeing it in both Nightly and Beta.

I can confirm Jeff's testing in Comment 6 - here is what I am seeing locally:

Nightly (38) - affected
Dev Edition (37) - affected
Beta (36) - affected
Release (35) - unaffected

Comment 10

[Brian Grinstead [:bgrins]]

To reproduce:

Open http://canuckistani.github.io/bug-1128747/
Open devtools Style Editor panel
Make sure "Show original sources" option is checked in the gear within the panel
Click on the main.scss file on the left

There is a big white space above the editor, as seen in attachment 8558197 [details].  There shouldn't be any extra space here.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

My mistake, reverting the flags to track in that case so we don't ship this regression.  We need an assignee here to make sure we get a backout or low risk forward fix into Beta asap. Jeff - is there someone you can get on this?

Comment 12

[Brian Grinstead [:bgrins]]

I will take it

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Brian, when are you planning to land a fix? Today is gtb for beta 8. Thanks

Comment 14

[Brian Grinstead [:bgrins]]

Tracked down regression window for nightlies

Good: 2014-11-13
Bad: 2014-11-14
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ab137ddd3746&tochange=7f0d92595432

Trying to narrow it down further seems to give different results each time.  I think the test case is only intermittently failing.  I've ran mozregression a few of times and gotten the same nightly each time, so I'm fairly confident in that range.

Comment 15

[Brian Grinstead [:bgrins]]

I have confirmed locally that backing out revision fd4bd4b3fbf8 from bug 1094208 causes the problem to go away.  Given that change [0] this looks like some kind of race condition that isn't covered by style editor tests.  I noticed while debugging that it seems like two separate sources are added to the style editor list (main.scss and main.css).

> when are you planning to land a fix? Today is gtb for beta 8.

I'm not going to have a fix ready today, I'm still trying to understand exactly why this is failing and come up with a testcase locally.

[0]: https://hg.mozilla.org/mozilla-central/rev/fd4bd4b3fbf8

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Brian, What about doing a backout of fd4bd4b3fbf8 ?

Comment 17

[:Paolo Amadini]

(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> Brian, What about doing a backout of fd4bd4b3fbf8 ?

We shouldn't back out this change except as a last resort for Beta, because this would make Promise.jsm behave differently from DOM Promises, and other parts of the code may now depend on the correct scheduling in Promise.jsm, so we'd have the risk of new regressions and race conditions elsewhere.

Going forward, we're going to remove Promise.jsm completely in favor of DOM promises, when the dependencies of bug 939636 are solved, so we shouldn't definitely back this change out in other channels.

Comment 18

[Brian Grinstead [:bgrins]]

I agree with Comment 17.  This is a pretty specific problem contained only within the Style Editor and only certain sourcemapped files.  I have a fix in place locally that resolves the problem, I just need to make sure that all other tests are passing.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

OK. FYI, the build of beta 9 will start Thursday. Please make sure it lands tomorrow.
Otherwise, we will either ship with this bug or disable this feature.
Thanks!

Comment 20

[Brian Grinstead [:bgrins]]

Attachment: splitview-sync-oncreate.patch

Jeff, can you confirm that this fixes your issue using the following build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=28eeed110b41?

Joe, I'm still trying to figure out how to test this (it's been harder to test than I was hoping).  But maybe you have some context on why this was done? (I found bug 583041 by looking through blame, but don't see it addressed specifically there).  The change in the promise implementation seems to have upset the timing here when the style editor tries to set a sheet as selected during the onCreate callback.

Comment 21

[Joe Walker [:jwalker]]

Comment on attachment 8562389 [details] [diff] [review]
splitview-sync-oncreate.patch

Review of attachment 8562389 [details] [diff] [review]:
-----------------------------------------------------------------

It looks from the original commit that this could have had something to do with animation [1]. "Only call onCreate when the animation has started". I have vague recollections of the animations being cool, but a constant source of problems. So this could be some hackery around that. The animation code is no longer in appendItem, so this could be a safe thing to remove.

[1]: https://hg.mozilla.org/integration/fx-team/diff/2513f713a23e/browser/devtools/styleeditor/SplitView.jsm

Comment 22

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Comment on attachment 8562389 [details] [diff] [review]
splitview-sync-oncreate.patch

Review of attachment 8562389 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Created attachment 8562389 [details] [diff] [review]
> splitview-sync-oncreate.patch
> 
> Jeff, can you confirm that this fixes your issue using the following build:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=28eeed110b41?

I can't reproduce with this build - yay! I noticed some other minor weirdness and logged bug 1132100 - I noticed that the main.scss file is not always selected / visible in the editor.

Comment 23

[Brian Grinstead [:bgrins]]

Attachment: styleeditor-load-error-testcase.patch

Test case.  Locally that this passes with the fix applied and fails without.  Waiting for a couple of try pushes to confirm.

New test with fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9035bd0ac31c
New test without fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67fd0c29290b

Comment 24

[Brian Grinstead [:bgrins]]

Attachment: splitview-sync-oncreate.patch

Tests show up just as expected, so folded the test changes into this patch.

Comment 25

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3877af16026

Comment 26

[Carsten Book [:Tomcat]]

Hi Brian,

this failed to apply:

renamed 1128747 -> splitview-sync-oncreate.patch
applying splitview-sync-oncreate.patch
patching file browser/devtools/styleeditor/test/browser.ini
Hunk #1 FAILED at 26
1 out of 2 hunks FAILED -- saving rejects to file browser/devtools/styleeditor/test/browser.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh splitview-sync-oncreate.patch

could you take a look, thanks!

Comment 27

[Brian Grinstead [:bgrins]]

Rebased and landed on fx-team:

https://hg.mozilla.org/integration/fx-team/rev/d74068ffe2ed

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d74068ffe2ed

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Brian, what are your plans wrt to this bug?

Comment 30

[Sylvestre Ledru [:Sylvestre]]

(for aurora & beta)

Comment 31

[Brian Grinstead [:bgrins]]

Going to request uplift for Aurora.  I'd like to give this some time to bake rather than request it land in Beta and get merged to release in the next week.

Comment 32

[Brian Grinstead [:bgrins]]

Comment on attachment 8563233 [details] [diff] [review]
splitview-sync-oncreate.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1094208
[User impact if declined]: The style editor code shows up in the wrong place (see attachment 8558197 [details])
[Describe test coverage new/current, TreeHerder]: There is a new test plus existing Style Editor tests are all passing
[Risks and why]: The problem was a timing issue with the style editor templating and promise resolutions so there is a risk that the fix caused some other problems within the Style Editor.  But nothing has surfaced so far, and all the tests are passing.  The risk is limited since it only affects the Style Editor tab inside of Dev Tools.
[String/UUID change made/needed]:

Comment 33

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8563233 [details] [diff] [review]
splitview-sync-oncreate.patch

This is a large patch but the actual fix is quite small. (The rest of the patch contains tests.) The risk here is limited in that this only affects the style editor, which is already in a bad state. Aurora+

Comment 34

[Sylvestre Ledru [:Sylvestre]]

(In reply to Brian Grinstead [:bgrins] from comment #31)
> Going to request uplift for Aurora.  I'd like to give this some time to bake
> rather than request it land in Beta and get merged to release in the next
> week.
Next week is going to be too late.
Today, we build beta 10, next Thursday, 36 RC.
So, it seems that we will ship with this bug.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c0692b67d2d