Closed Bug 828176 Opened 7 years ago Closed 7 years ago

Make RasterImage::GetURIString work again

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: justin.lebar+bug, Assigned: seth)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
It currently always returns the empty string.
Proposed patch to add GetURIString() back, with a different signature. I'm not
wild about this fix since we already have GetURI(), but for now this is the
simplest solution due to the use of this function in sample labels. If we
forced the use of GetURI() we'd have to have appropriate #ifdef's around the
code that calls GetURI()->GetSpec(), since GetSpec() is an XPCOM function and
can't be rolled into the same expression as the sample label. 

Long term the right solution is to add a non-XPCOM version of GetSpec() to
nsIURI that returns an nsCString normally instead of through an out parameter.
That would eliminate the need for GetURIString() totally, and would probably be
useful in other places as well.
Attachment #700026 - Flags: review?(joe)
Assignee: nobody → seth
Comment on attachment 700026 [details] [diff] [review]
Make RasterImage::GetURIString work again.

> +  SAMPLE_LABEL_PRINTF("RasterImage", "SyncDecode", "%s", GetURIString().get());;

Does this work?  I expect that %s expects a char*, not a PRUnichar*.

> +  nsString GetURIString() {

I don't know if this is kosher, but if it works, that's great.
Attachment #700026 - Flags: review?(joe) → review+
I believe that returning an nsString by value is just fine. You're right that SAMPLE_LABEL_PRINTF wants a const char*, though. Sorry for the oversight. I'll move the conversion to UTF16 back out of GetURIString(), which will resolve the issue. Will post an updated patch shortly.
(The original code didn't handle this correctly either BTW, although the error was in a different place. Gotta love text encodings.)
Updated patch with fixes from review.
Attachment #700026 - Attachment is obsolete: true
Looks like a null pointer dereference in that last push. I've added a check for a null GetURI() in GetURIString(). New try job here:

https://tbpl.mozilla.org/?tree=Try&rev=6f2805c21fb4
OK, I think we're good to go here. Try looks fine. Requesting checkin.
Keywords: checkin-needed
Backed out for crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/838146e5e46a

https://tbpl.mozilla.org/php/getParsedLog.php?id=19298820&tree=Mozilla-Inbound

TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_bug395533.html | Exited with code 1 during test run
INFO | automation.py | Application ran for: 0:00:41.478997
INFO | automation.py | Reading PID log: /var/folders/Hs/HsDn6a9SG8idoIya6p9mtE+++TI/-Tmp-/tmp0ccQeBpidlog
PROCESS-CRASH | /tests/browser/base/content/test/test_bug395533.html | application crashed [@ mozilla::image::RasterImage::SyncDecode()]
Crash dump filename: /var/folders/Hs/HsDn6a9SG8idoIya6p9mtE+++TI/-Tmp-/tmp0Wv_NJ/minidumps/A22234CE-683E-42E0-A864-978612CDBDAE.dmp
Operating system: Mac OS X
                  10.6.8 10K549
CPU: amd64
     family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0

Thread 0 (crashed)
 0  XUL!mozilla::image::RasterImage::SyncDecode() [RasterImage.h:25ea9ac37380 : 295 + 0xc]
    rbx = 0x000000014b677c60   r12 = 0x00007fff5fbfcbf8
    r13 = 0x000000014b677c60   r14 = 0x0000000000000000
    r15 = 0x00007fff5fbfcbd8   rip = 0x00000001012a99fe
    rsp = 0x00007fff5fbfcb40   rbp = 0x00007fff5fbfcc60
    Found by: given as instruction pointer in context
 1  XUL!mozilla::image::RasterImage::CopyFrame(unsigned int, unsigned int, gfxImageSurface**) [RasterImage.cpp:25ea9ac37380 : 994 + 0x7]
    rbx = 0x0000000000000000   r12 = 0x0000000000000001
    r13 = 0x000000014b677c60   r14 = 0x0000000000000001
    r15 = 0x0000000080070057   rip = 0x00000001012aa3cd
    rsp = 0x00007fff5fbfcc70   rbp = 0x00007fff5fbfcd90
    Found by: call frame info
 2  XUL!imgTools::GetFirstImageFrame(imgIContainer*, gfxImageSurface**) [imgTools.cpp:25ea9ac37380 : 265 + 0xe]
    rbx = 0x0000000000000010   r12 = 0x0000000000000010
    r13 = 0x000000014b677c60   r14 = 0x00007fff5fbfceb0
    r15 = 0x0000000000000010   rip = 0x00000001012d2da8
    rsp = 0x00007fff5fbfcda0   rbp = 0x00007fff5fbfcdc0
    Found by: call frame info
 3  XUL!imgTools::EncodeScaledImage(imgIContainer*, nsACString_internal const&, int, int, nsAString_internal const&, nsIInputStream**) [imgTools.cpp:25ea9ac37380 : 134 + 0x4]
    rbx = 0x0000000000000010   r12 = 0x0000000000000010
    r13 = 0x000000014b677c60   r14 = 0x000000014b63b030
    r15 = 0x0000000000000010   rip = 0x00000001012d31cd
    rsp = 0x00007fff5fbfcdd0   rbp = 0x00007fff5fbfcee0
    Found by: call frame info
 4  XUL!nsFaviconService::OptimizeFaviconImage(unsigned char const*, unsigned int, nsACString_internal const&, nsACString_internal&, nsACString_internal&) [nsFaviconService.cpp:25ea9ac37380 : 1070 + 0x1c]
    rbx = 0x0000000000000000   r12 = 0x0000000000000010
    r13 = 0x000000014b677c60   r14 = 0x000000014b63b030
Whoops. Foolishly forgot to upload the updated version of the patch (which has an all-green try run, as seen above).
Attachment #700805 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2f42e4ce2f5c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.