Closed Bug 993548 Opened 6 years ago Closed 6 years ago
aggressively free source string in ns
Script Loader::On Stream Complete()
This is the patch I am currently testing with for bug 987556. I'm not thrilled with the const_cast, but not sure about if it would be preferable to change the IDL to not require const on the string here.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attachment #8403465 - Flags: review?(khuey)
Nom for 1.3? since we need this for bug 987556 which is a 1.3+ blocker.
blocking-b2g: --- → 1.3?
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
Comment on attachment 8403465 [details] [diff] [review] Free load buffer before starting script compile. (v1) Review of attachment 8403465 [details] [diff] [review]: ----------------------------------------------------------------- NS_Free is what the font loader uses, so I guess it's ok.
Attachment #8403465 - Flags: review?(khuey) → review+
I filed bug 993591 on a more general solution.
Try is showing a persistent failure on Android 4.0 Opt RC2, but that appears to have been a separate problem just fixed in e2d52a541b50. I'm going to proceed with landing this on mozilla-inbound.
Ben - Can you give a risk assessment of this patch? Just trying to find out if we can safely take this to help resolve bug 987556 or not.
This is very low risk and our automated testing of Gecko will cover it well. If it sticks on trunk we should absolutely take it on the branch.
(In reply to Jason Smith [:jsmith] from comment #7) > Ben - Can you give a risk assessment of this patch? Just trying to find out > if we can safely take this to help resolve bug 987556 or not. I believe this is low risk. We are essentially just moving up the release from the destructor in nsStreamLoader up one layer to nsScriptLoader. The ability to pass buffers on to the next layer for ownership already existed. So its really quite a small change.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Please request approval-mozilla-b2g28? on this patch when it is ready for v1.3 uplift. Also, is this something we should consider taking on mozilla-beta for Fx29?
Comment on attachment 8403465 [details] [diff] [review] Free load buffer before starting script compile. (v1) [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 987556 User impact if declined: We use 14MB of additional memory trying to run browsermark which is required for partner IOT. Testing completed: Device tested on v1.5 and v1.3. Risk to taking this patch (and alternatives if risky): Minimal String or UUID changes made by this patch: None
Attachment #8403465 - Flags: approval-mozilla-b2g28?
I don't think this is worth uplifting to mozilla-beta since transient memory spikes do not have the same impact on resource rich environments like desktop.
Since I think triages are focusing more on 1.4 and tarako these days, I'm going to do a ni? to request someone look at the approval flag.
Comment on attachment 8403465 [details] [diff] [review] Free load buffer before starting script compile. (v1) Also a=bajaj to land it on aurora.
Attachment #8403465 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
(In reply to bhavana bajaj [:bajaj] from comment #16) > Also a=bajaj to land it on aurora. Did you mean b2g28 here? I would like this for the linked 1.3+ blocked bug.
You need to log in before you can comment on or make changes to this bug.