713359 - CSS cut short in Style Editor

Status: VERIFIED FIXED

(DevTools :: Style Editor, defect, P2)

Description

[Stewart Souter]

Attachment: StyleEditor.jpg

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:12.0a1) Gecko/20111223 Firefox/12.0a1
Build ID: 20111223032107

Steps to reproduce:

Just opened the Style Editor


Actual results:

the CSS displayed in the Style Editor is cut short or not being fully loaded. If you visit one of my sites, http://carbonize.co.uk/Lazarus/ and open style editor you will see that the code from style.css is not all there.


Expected results:

All the CSS from the file should be visible

Comment 1

[Paul Rouget [:paul]]

I can reproduce. Cedric, can you please take a look at this?

Comment 2

[:Felipe Gomes (needinfo for replies!)]

I can also reproduce this bug in robcee's website: http://antennasoft.net/robcee/

Comment 3

[Cedric Vivier [:cedricv]]

Attachment: Fix

I'm still unsure why the issue was triggered on some content and not others, hence the current lack of regression test [it seems to always pass when served through the mochitest web server].

Comment 4

[Dave Camp (:dcamp)]

Comment on attachment 586101 [details] [diff] [review]
Fix

So you're treating an async IO stream synchronously.  available() doesn't really mean "number of bytes left before EOF" it means more like "number of bytes that have been read off the disk already in the IO thread".  So if you spin a loop waiting for number of read bytes to equal content-length, you're really just blocking the main thread long enough for the IO thread to read the whole file (which is bad).  This should be redone as a real async load.

Looking a bit further up, you're manually constructing a stream converter based on headers.  This isn't particularly maintainable.  Also, this seems to only work on things that were cached.

It seems like what you really want to do is construct an HTTP channel with the proper flags to prefer the cached resource, and let it figure out the correct encoding/caching semantics.

I can throw together a quick patch for this later today.

Comment 5

[Cedric Vivier [:cedricv]]

Attachment: Fix v2 - refactor using nsIChannel w/ LOAD_FROM_CACHE instead of manual nsIStreamConverter from cache

Wow. Thanks for that enlightenment, now makes much more sense :)

Tentative v2.

Comment 6

[Rob Campbell [:rc] (:robcee)]

is this ready to land?

Comment 7

[Cedric Vivier [:cedricv]]

Yes. And we'll likely need to later ask for beta approval as this is an important fix.

Comment 8

[Rob Campbell [:rc] (:robcee)]

(In reply to Cedric Vivier [cedricv] from comment #7)
> Yes. And we'll likely need to later ask for beta approval as this is an
> important fix.

should just need it in aurora as style editor isn't in beta yet, but yes! Most definitely.

Comment 9

[Alex Keybl [:akeybl]]

Please nominate for Aurora approval once this has had a couple of days of bake time on m-c.

Comment 10

[Rob Campbell [:rc] (:robcee)]

(In reply to Alex Keybl [:akeybl] from comment #9)
> Please nominate for Aurora approval once this has had a couple of days of
> bake time on m-c.

that was the plan.

Comment 11

[Rob Campbell [:rc] (:robcee)]

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

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 13

[Alex Lakatos[:AlexLakatos]]

Verified on Build identifier: Mozilla/5.0 (X11; Linux i686; rv:12.0a1) Gecko/20120125 Firefox/12.0a1

Comment 14

[Dave Camp (:dcamp)]

Comment on attachment 586116 [details] [diff] [review]
Fix v2 - refactor using nsIChannel w/ LOAD_FROM_CACHE instead of manual nsIStreamConverter from cache

[Approval Request Comment]
Regression caused by (bug #): New feature
User impact if declined: Nasty bug cutting CSS short.
Testing completed (on m-c, etc.): Been on m-c for almost a month.
Risk to taking this patch (and alternatives if risky): Honestly can't think of anything worse than the bug it fixes.  Code is simpler than the code it's replacing.
String changes made by this patch: None.

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 586116 [details] [diff] [review]
Fix v2 - refactor using nsIChannel w/ LOAD_FROM_CACHE instead of manual nsIStreamConverter from cache

[Triage Comment]
Low risk and a poor user experience - approved for Aurora 12 and Beta 11.

Comment 16

[Dave Camp (:dcamp)]

https://hg.mozilla.org/releases/mozilla-beta/rev/57312e22d36c

Comment 17

[Dave Camp (:dcamp)]

This landed Jen16, before the fx12 aurora cutoff, so it's already on aurora.

Comment 18

[Simona Badau, Desktop QA]

Verified using the pages provided in the Description and Comment 2 - the CSS displayed in the Style Editor is complete.

Tested using Firefox 11 beta 4 on Windows 7, Ubuntu 11.10 and Mac OS X 10.6:

Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0

Comment 19

[Simona Badau, Desktop QA]

Verified as fixed on Firefox 12 beta 2 using the same steps from Comment 18 - the CSS displayed in the Style Editor is complete.

Verified on Windows 7, Ubuntu 11.10 and Mac OS X 10.6.
Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0

Setting resolution to VERIFIED FIXED.

Comment 20

[Sztukateria]

I have used it on my page http://decorsystem.com.pl