Closed
Bug 813818
Opened 12 years ago
Closed 12 years ago
GfxInfoBase::{Model,Hardware,Product,Manufacturer}() return the address of a temporary, triggering warnings and undefined behavior
Categories
(Core :: Widget, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: Waldo, Assigned: bjacob)
References
Details
(Keywords: regression)
Attachments
(3 files)
4.86 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
1.43 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
3.12 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
Not cool.
Assignee | ||
Comment 1•12 years ago
|
||
Sorry about that. Looking.
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #683845 -
Flags: review?(joe)
Assignee | ||
Updated•12 years ago
|
Attachment #683845 -
Attachment is patch: true
Comment 3•12 years ago
|
||
Comment on attachment 683845 [details] [diff] [review] aint benoit funny with his clown shoes i am the worst reviewer in the world
Attachment #683845 -
Flags: review?(joe) → review+
Comment 4•12 years ago
|
||
Wait, how is that a temporary?
Comment 5•12 years ago
|
||
Could you make the return types nsString and save us the heartache of GetFoo(blah)?
Updated•12 years ago
|
status-firefox17:
--- → affected
status-firefox18:
--- → affected
status-firefox19:
--- → affected
status-firefox20:
--- → affected
Keywords: regression
OS: All → Android
Hardware: All → ARM
Version: Trunk → 17 Branch
Comment 6•12 years ago
|
||
Is return EmptyString(); safe?
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #6) > Is return EmptyString(); safe? This looks like it works! http://mxr.mozilla.org/mozilla-central/source/xpcom/string/src/nsReadableUtils.cpp#998 998 static const PRUnichar empty_buffer[1] = { '\0' }; 999 1000 const nsAFlatString& 1001 EmptyString() 1002 { 1003 static const nsDependentString sEmpty(empty_buffer); 1004 1005 return sEmpty; 1006 } So there is no temporary there.
Assignee | ||
Comment 8•12 years ago
|
||
The only concern here is that in nsStringAPI.h, which we *should* not be using here, EmptyString() is just a macro for nsString(). Anyway, this built without warning.
Attachment #684104 -
Flags: review?(joe)
Updated•12 years ago
|
Attachment #684104 -
Flags: review?(joe) → review+
Reporter | ||
Comment 9•12 years ago
|
||
Isn't that a static initializer?
Reporter | ||
Comment 10•12 years ago
|
||
As another option, why not just return an nsString directly? RVO is going to save the copy you're presumably trying to avoid.
Reporter | ||
Comment 11•12 years ago
|
||
Or, hm, virtual, maybe not.
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #684454 -
Flags: review?(joe)
Updated•12 years ago
|
Attachment #684454 -
Flags: review?(joe) → review+
Assignee | ||
Comment 13•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/d60228a6f869 http://hg.mozilla.org/integration/mozilla-inbound/rev/d7708795f3b4
Assignee: nobody → bjacob
Target Milestone: --- → mozilla20
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d60228a6f869 https://hg.mozilla.org/mozilla-central/rev/d7708795f3b4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
status-firefox20:
affected → ---
Updated•11 years ago
|
Comment 15•11 years ago
|
||
Benoit - is there any benefit to taking this in FF19, even though we'll likely be blocked on bug 838845 before using mobile graphics blocklisting again?
Assignee | ||
Comment 16•11 years ago
|
||
We don't seem to have any android rule left in blocklist.xml, so I suppose that the answer is no. But if we have (i only looked at the copy in my desktop profile, dont know how conclusive this is), then we need this.
Flags: needinfo?(bjacob)
Comment 17•11 years ago
|
||
There are indeed no Android rules in the current blocklist.xml, common to desktop and mobile, but we won't be able to fix top crashers related to Stagefright (e.g. bug 802827 and bug 808378) in 19.0. Bug 838845 is tracked for 19.0 so this one should be too.
Comment 18•11 years ago
|
||
We're going to be performing the review in bug 838845 in the FF20 timeframe. Since we're blocked on that anyway, I don't think we should take any new change here in our final beta.
tracking-firefox20:
--- → +
You need to log in
before you can comment on or make changes to this bug.
Description
•