56.26 KB, image/jpeg
Fix v2 - refactor using nsIChannel w/ LOAD_FROM_CACHE instead of manual nsIStreamConverter from cache
3.81 KB, patch
|Details | Diff | Splinter Review|
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
I can reproduce. Cedric, can you please take a look at this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Version: 12 Branch → Trunk
I can also reproduce this bug in robcee's website: http://antennasoft.net/robcee/
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].
Attachment #586101 - Flags: review?
Attachment #586101 - Flags: review? → review?(rcampbell)
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.
Attachment #586101 - Flags: review?(rcampbell) → review-
Wow. Thanks for that enlightenment, now makes much more sense :) Tentative v2.
is this ready to land?
Status: NEW → ASSIGNED
Yes. And we'll likely need to later ask for beta approval as this is an important fix.
Whiteboard: [land-in-fx-team?] → [land-in-fx-team]
(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.
Please nominate for Aurora approval once this has had a couple of days of bake time on m-c.
(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.
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Verified on Build identifier: Mozilla/5.0 (X11; Linux i686; rv:12.0a1) Gecko/20120125 Firefox/12.0a1
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 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.
This landed Jen16, before the fx12 aurora cutoff, so it's already on aurora.
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
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.
You need to log in before you can comment on or make changes to this bug.