Last Comment Bug 713359 - CSS cut short in Style Editor
: CSS cut short in Style Editor
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Style Editor (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 12
Assigned To: Cedric Vivier [:cedricv]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-24 05:08 PST by Stewart Souter
Modified: 2012-03-23 05:47 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
verified


Attachments
StyleEditor.jpg (56.26 KB, image/jpeg)
2011-12-24 05:08 PST, Stewart Souter
no flags Details
Fix (1.98 KB, patch)
2012-01-05 08:47 PST, Cedric Vivier [:cedricv]
dcamp: review-
Details | Diff | Review
Fix v2 - refactor using nsIChannel w/ LOAD_FROM_CACHE instead of manual nsIStreamConverter from cache (3.81 KB, patch)
2012-01-05 09:39 PST, Cedric Vivier [:cedricv]
dcamp: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Stewart Souter 2011-12-24 05:08:46 PST
Created attachment 584188 [details]
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] 2012-01-03 03:32:28 PST
I can reproduce. Cedric, can you please take a look at this?
Comment 2 :Felipe Gomes (needinfo me!) 2012-01-04 09:27:34 PST
I can also reproduce this bug in robcee's website: http://antennasoft.net/robcee/
Comment 3 Cedric Vivier [:cedricv] 2012-01-05 08:47:56 PST
Created attachment 586101 [details] [diff] [review]
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) 2012-01-05 09:09:43 PST
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] 2012-01-05 09:39:18 PST
Created attachment 586116 [details] [diff] [review]
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) 2012-01-10 06:41:06 PST
is this ready to land?
Comment 7 Cedric Vivier [:cedricv] 2012-01-11 01:19:39 PST
Yes. And we'll likely need to later ask for beta approval as this is an important fix.
Comment 8 Rob Campbell [:rc] (:robcee) 2012-01-11 09:33:12 PST
(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] 2012-01-12 12:56:32 PST
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) 2012-01-12 14:48:24 PST
(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) 2012-01-14 09:58:20 PST
https://hg.mozilla.org/integration/fx-team/rev/fb9e08e72815
Comment 12 Tim Taubert [:ttaubert] 2012-01-16 02:30:08 PST
https://hg.mozilla.org/mozilla-central/rev/fb9e08e72815
Comment 13 Alex Lakatos[:AlexLakatos] 2012-01-25 07:31:26 PST
Verified on Build identifier: Mozilla/5.0 (X11; Linux i686; rv:12.0a1) Gecko/20120125 Firefox/12.0a1
Comment 14 Dave Camp (:dcamp) 2012-02-13 12:52:04 PST
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] 2012-02-13 17:19:19 PST
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 17 Dave Camp (:dcamp) 2012-02-14 11:38:27 PST
This landed Jen16, before the fx12 aurora cutoff, so it's already on aurora.
Comment 18 Simona B [:simonab] 2012-02-24 05:44:34 PST
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 B [:simonab] 2012-03-23 05:47:23 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.