Closed Bug 813818 Opened 8 years ago Closed 8 years ago

GfxInfoBase::{Model,Hardware,Product,Manufacturer}() return the address of a temporary, triggering warnings and undefined behavior

Categories

(Core :: Widget, defect)

17 Branch
ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox17 --- affected
firefox18 - wontfix
firefox19 - affected
firefox20 + fixed

People

(Reporter: Waldo, Assigned: bjacob)

References

Details

(Keywords: regression)

Attachments

(3 files)

Not cool.
Sorry about that. Looking.
Attachment #683845 - Attachment is patch: true
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+
Wait, how is that a temporary?
Could you make the return types nsString and save us the heartache of GetFoo(blah)?
Keywords: regression
OS: All → Android
Hardware: All → ARM
Version: Trunk → 17 Branch
Is return EmptyString(); safe?
(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.
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)
Attachment #684104 - Flags: review?(joe) → review+
Isn't that a static initializer?
As another option, why not just return an nsString directly?  RVO is going to save the copy you're presumably trying to avoid.
Or, hm, virtual, maybe not.
Attachment #684454 - Flags: review?(joe) → review+
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?
Flags: needinfo?(bjacob)
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)
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.
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.
You need to log in before you can comment on or make changes to this bug.