Bug 67582

Summary: Calling nativeImageForCurrentFrame() causes assertion failure: m_verifier.isSafeToUse()
Product: WebKit Reporter: Huajun.Li <huajun.li.lee>
Component: WebCore Misc.Assignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, aroben, beidson, cmarcelo, darin, dongseong.hwang, gyuyoung.kim, hyuki.kim, japhet, jochen, leandro, levin, levin+threading, menard, naginenis, rakuco, s.choi, svillar, tonikitoo, webkit.review.bot, yong.li.webkit, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
buildbot: commit-queue-
addition to the proposed patch
none
Merged together and try to fix windows build
none
Try again.
buildbot: commit-queue-
Try fixing the build error (also removed change in webkit/cf)
buildbot: commit-queue-
put WTF_USE_CG back, this should build now levin: review+

Description Huajun.Li 2011-09-04 19:41:26 PDT
Reproduce steps:
1. Launch EWebLauncher with cmd  'build/Programs/EWebLauncher  -v'
2. Surfing some web pages
3. Press 'F1' to try to backward the page, you will find the browser segment fault, output msg likes "ASSERTION FAILED: m_verifier.isSafeToUse()
../Source/JavaScriptCore/wtf/RefCounted.h(53) : void WTF::RefCountedBase::ref()"

I am using git://gitorious.org/webkit/webkit.git, commit 0c5c7a3aaebfbd5451df634292fe182ee99e5a5d


Below is the backtrace :
-----------------------------------------------------------
(gdb) c
Continuing.
[New Thread 0x7f1daf02a700 (LWP 15960)]
[Thread 0x7f1daf02a700 (LWP 15960) exited]
[New Thread 0x7f1daf02a700 (LWP 15961)]
[Thread 0x7f1daf02a700 (LWP 15961) exited]
[New Thread 0x7f1daf02a700 (LWP 15962)]
[Thread 0x7f1daf02a700 (LWP 15962) exited]

Program received signal SIGSEGV, Segmentation fault.
0x00007f1dc0acea94 in WTF::RefCountedBase::ref (this=0x22d1400)
    at /home/huajun/build_webkit_repo/Source/JavaScriptCore/wtf/RefCounted.h:53
53              ASSERT(m_verifier.isSafeToUse());
(gdb) bt
#0  0x00007f1dc0acea94 in WTF::RefCountedBase::ref (this=0x22d1400)
    at /home/huajun/build_webkit_repo/Source/JavaScriptCore/wtf/RefCounted.h:53
#1  0x00007f1dbdc10d8a in WTF::refIfNotNull<WebCore::SharedBuffer> (ptr=0x22d1400)
    at /home/huajun/build_webkit_repo/Source/JavaScriptCore/wtf/PassRefPtr.h:53
#2  0x00007f1dbddbc6f8 in WTF::RefPtr<WebCore::SharedBuffer>::operator= (this=0x2309e70, optr=0x22d1400)
    at /home/huajun/build_webkit_repo/Source/JavaScriptCore/wtf/RefPtr.h:132
#3  0x00007f1dbe6d2765 in WebCore::BMPImageReader::setData (this=0x2309e60, data=0x22d1400)
    at /home/huajun/build_webkit_repo/Source/WebCore/platform/image-decoders/bmp/BMPImageReader.h:72
#4  0x00007f1dbe6db24e in WebCore::ICOImageDecoder::decodeAtIndex (this=0x1c2ccd0, index=0)
    at /home/huajun/build_webkit_repo/Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:196
#5  0x00007f1dbe6daee0 in WebCore::ICOImageDecoder::decode (this=0x1c2ccd0, index=0, onlySize=false)
    at /home/huajun/build_webkit_repo/Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:161
#6  0x00007f1dbe6dac7a in WebCore::ICOImageDecoder::frameBufferAtIndex (this=0x1c2ccd0, index=0)
    at /home/huajun/build_webkit_repo/Source/WebCore/platform/image-decoders/ico/ICOImageDecoder.cpp:121
#7  0x00007f1dbe6cf9da in WebCore::ImageSource::createFrameAtIndex (this=0x22f5158, index=0)
    at /home/huajun/build_webkit_repo/Source/WebCore/platform/graphics/ImageSource.cpp:138
#8  0x00007f1dbdef1477 in WebCore::BitmapImage::cacheFrame (this=0x22f5120, index=0)
    at /home/huajun/build_webkit_repo/Source/WebCore/platform/graphics/BitmapImage.cpp:127
#9  0x00007f1dbdef1b46 in WebCore::BitmapImage::frameAtIndex (this=0x22f5120, index=0)
    at /home/huajun/build_webkit_repo/Source/WebCore/platform/graphics/BitmapImage.cpp:248
#10 0x00007f1dbdef256e in WebCore::BitmapImage::nativeImageForCurrentFrame (this=0x22f5120)
    at /home/huajun/build_webkit_repo/Source/WebCore/platform/graphics/BitmapImage.h:160
#11 0x00007f1dbb82690b in ewk_history_item_icon_surface_get (item=0x24ead50)
    at /home/huajun/build_webkit_repo/Source/WebKit/efl/ewk/ewk_history.cpp:363
#12 0x00000000004030d6 in print_history (list=0x1a7db90) at /home/huajun/build_webkit_repo/Tools/EWebLauncher/main.c:177
#13 0x0000000000403fb0 in on_key_down (data=0x1ac34b0, e=0x1ad77f0, obj=0x7f1dc337b200, event_info=0x7fff3f985b90)
    at /home/huajun/build_webkit_repo/Tools/EWebLauncher/main.c:515
#14 0x00007f1dc2806842 in evas_object_event_callback_call (obj=0x7f1dc337b200, type=EVAS_CALLBACK_KEY_DOWN, event_info=0x7fff3f985b90)
    at evas_callbacks.c:224
#15 0x00007f1dc280e40b in evas_event_feed_key_down (e=0x1ad77f0, keyname=<value optimized out>, key=<value optimized out>,
    string=<value optimized out>, compose=<value optimized out>, timestamp=<value optimized out>, data=0x0) at evas_events.c:1249
#16 0x00007f1db5891bf4 in _ecore_event_evas_key (e=0x21ee280, press=ECORE_DOWN) at ecore_input_evas.c:153
#17 0x00007f1dc25c41a5 in _ecore_call_handler_cb () at ecore_private.h:287
#18 _ecore_event_call () at ecore_events.c:691
#19 0x00007f1dc25c89a5 in _ecore_main_loop_iterate_internal (once_only=0) at ecore_main.c:1746
#20 0x00007f1dc25c8faf in ecore_main_loop_begin () at ecore_main.c:861
#21 0x0000000000405640 in main (argc=2, argv=0x7fff3f986ec8) at /home/huajun/build_webkit_repo/Tools/EWebLauncher/main.c:914
(gdb)
Comment 1 David Levin 2011-09-05 23:26:27 PDT
So the problem is her: http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/image-decoders/ImageDecoder.h&type=cs&l=246

In ImageDecoder.h
        virtual void setData(SharedBuffer* data, bool allDataReceived)
        {
            if (m_failed)
                return;
            m_data = data;
On this last line, which ref counts a shared buffer.

That shared buffer is part of the icon which is shared across threads. At the moment, I don't know if something really tricky is going on or this is not thread safe -- at the moment, it doesn't look threadsafe.
Comment 2 Darin Adler 2011-09-06 09:05:40 PDT
I think the right question for us is what is the design. What is supposed to make this threadsafe?
Comment 3 David Levin 2011-09-06 13:35:19 PDT
(In reply to comment #2)
> I think the right question for us is what is the design. What is supposed to make this threadsafe?

There are two comments in the code to indicate the design:
1. From IconDatabase.h,  "Holding m_urlAndIconLock is required when accessing any of the following data structures or the objects they contain". This includes the data that is refcounted in this callstack. The lock is not held so the assert is correct in that sense.

2. In  IconDatabase::synchronousIconForPageURL, there is a comment which starts with "PARANOID DISCUSSION" (added here http://trac.webkit.org/changeset/25439/trunk/WebCore/loader/icon/IconDatabase.cpp). It has a long explanation about why returning Image* is ok. I don't think it considered platforms which did ref counting of the guts of these structures.

In short, I believe this is real a bug.

At a high level the appropriate fix would be to lock m_urlAndIconLock when accessing the SharedBuffer or for synchronousIconForPageURL to return a PassRefPtr which isn't shared across threads..
Comment 4 David Levin 2011-09-08 10:55:38 PDT
Adding some people who seem to have been involved in the ewk api since it looks like the change needs to happen there.
Comment 5 Raphael Kubo da Costa (:rakuco) 2011-09-09 06:42:20 PDT
Sorry, I still fail to see what could be done in ewk.

ewk_history_item_icon_surface_get's behavior follows quite closely what the PARANOID DISCUSSION comment describes, in that it just calls synchronousIconForPageURL and creates a platform-specific representation of the raw image (in ewk's case, a cairo_surface_t). We can't hold the m_urlAndIconLock mutex ourselves, and the rest of the call stack is inside WebCore.

Can you elaborate a little on your thoughts?
Comment 6 David Levin 2011-09-09 09:16:05 PDT
(In reply to comment #5)
> Sorry, I still fail to see what could be done in ewk.
> 
> ewk_history_item_icon_surface_get's behavior follows quite closely what the PARANOID DISCUSSION comment describes,

The PARANOID DISCUSSION comment seems to make some assumptions that aren't always true. I don't think this method is safe for you to call like this since thing you do right after the call isn't safe.

imo, it would be hard to see this issue when that comment was written. It seems to assume a model like what windows does which calls "BitmapImage::getHBITMAPOfSize" This method doesn't ref count any internal structures.


> in that it just calls synchronousIconForPageURL and creates a platform-specific representation of the raw image (in ewk's case, a cairo_surface_t). We can't hold the m_urlAndIconLock mutex ourselves, and the rest of the call stack is inside WebCore.
> 
> Can you elaborate a little on your thoughts?

Your comments point in the right direction. Remove the call to IconDatabase ::synchronousIconForPageURL and add a new method to converts to your platform specific representation while holding the lock. Then call this method instead.

Does that make sense to you? (The current state of things appears to leave you open to race conditions which make cause misc crashes at random points in your code.)

PS imo, this method seems flawed and perhaps we should just remove it in every platform, but for this bug ewk is the target :). I suspect the OS X platform may have similar issues because it calls webGetNSImage which does some complicated things. I'll look at this for other platforms and file some bugs.
Comment 7 Huajun.Li 2011-09-09 18:21:53 PDT
(In reply to comment #6)
> > in that it just calls synchronousIconForPageURL and creates a platform-specific representation of the raw image (in ewk's case, a cairo_surface_t). We can't hold the m_urlAndIconLock mutex ourselves, and the rest of the call stack is inside WebCore.
> > 
> > Can you elaborate a little on your thoughts?
> 
> Your comments point in the right direction. Remove the call to IconDatabase ::synchronousIconForPageURL and add a new method to converts to your platform specific representation while holding the lock. Then call this method instead.
> 
> Does that make sense to you? (The current state of things appears to leave you open to race conditions which make cause misc crashes at random points in your code.)
> 
> PS imo, this method seems flawed and perhaps we should just remove it in every platform, but for this bug ewk is the target :). I suspect the OS X platform may have similar issues because it calls webGetNSImage which does some complicated things. I'll look at this for other platforms and file some bugs.

I ever checked QT port, and found it may have same issue, so it is not specific to ewk. :)
Comment 8 Sergio Villar Senin 2012-02-16 08:22:47 PST
(In reply to comment #7)
> (In reply to comment #6)
> > > in that it just calls synchronousIconForPageURL and creates a platform-specific representation of the raw image (in ewk's case, a cairo_surface_t). We can't hold the m_urlAndIconLock mutex ourselves, and the rest of the call stack is inside WebCore.
> > > 
> > > Can you elaborate a little on your thoughts?
> > 
> > Your comments point in the right direction. Remove the call to IconDatabase ::synchronousIconForPageURL and add a new method to converts to your platform specific representation while holding the lock. Then call this method instead.
> > 
> > Does that make sense to you? (The current state of things appears to leave you open to race conditions which make cause misc crashes at random points in your code.)
> > 
> > PS imo, this method seems flawed and perhaps we should just remove it in every platform, but for this bug ewk is the target :). I suspect the OS X platform may have similar issues because it calls webGetNSImage which does some complicated things. I'll look at this for other platforms and file some bugs.
> 
> I ever checked QT port, and found it may have same issue, so it is not specific to ewk. :)

And I also hit this bug in Gtk port when implementing some unit tests for our icon database client
Comment 9 David Levin 2012-02-16 08:25:51 PST
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > > in that it just calls synchronousIconForPageURL and creates a platform-specific representation of the raw image (in ewk's case, a cairo_surface_t). We can't hold the m_urlAndIconLock mutex ourselves, and the rest of the call stack is inside WebCore.
> > > > 
> > > > Can you elaborate a little on your thoughts?
> > > 
> > > Your comments point in the right direction. Remove the call to IconDatabase ::synchronousIconForPageURL and add a new method to converts to your platform specific representation while holding the lock. Then call this method instead.
> > > 
> > > Does that make sense to you? (The current state of things appears to leave you open to race conditions which make cause misc crashes at random points in your code.)
> > > 
> > > PS imo, this method seems flawed and perhaps we should just remove it in every platform, but for this bug ewk is the target :). I suspect the OS X platform may have similar issues because it calls webGetNSImage which does some complicated things. I'll look at this for other platforms and file some bugs.
> > 
> > I ever checked QT port, and found it may have same issue, so it is not specific to ewk. :)
> 
> And I also hit this bug in Gtk port when implementing some unit tests for our icon database client

It indicates that there is likely a problem in the code (maybe for multiple platforms), and it needs a volunteer to fix it.
Comment 10 Sergio Villar Senin 2012-02-16 08:59:55 PST
(In reply to comment #9)
> > And I also hit this bug in Gtk port when implementing some unit tests for our icon database client
> 
> It indicates that there is likely a problem in the code (maybe for multiple platforms), and it needs a volunteer to fix it.

Well, unless I'm missing something it's pretty clear to me that IconDatabase ::synchronousIconForPageURL cannot be used by any platform as it requires a lock in the mutex used to verify refcounting. This issue was likely never addressed as 1- it does not obviously affect release builds and 2- the code is kind of correct, because even if you don't get the lock, nothing wrong will happen as long as you call it from the same thread that created the image data.

One possible fix off the top of my head would be replacing the last return by a call to some method in the IconDatabaseClient that will get the Image and will return the platform specific image. Does it sound good?
Comment 11 David Levin 2012-02-16 09:04:32 PST
(In reply to comment #10)
> (In reply to comment #9)
> > > And I also hit this bug in Gtk port when implementing some unit tests for our icon database client
> > 
> > It indicates that there is likely a problem in the code (maybe for multiple platforms), and it needs a volunteer to fix it.
> 
> Well, unless I'm missing something it's pretty clear to me that IconDatabase ::synchronousIconForPageURL cannot be used by any platform as it requires a lock in the mutex used to verify refcounting. This issue was likely never addressed as 1- it does not obviously affect release builds and 2- the code is kind of correct, because even if you don't get the lock, nothing wrong will happen as long as you call it from the same thread that created the image data.

I agree with you mostly. (I don't think 2 is true but I haven't reviewed the code recently. iirc, there is a reason for the lock and verifying that ref counting only happens within the lock. When you do ref counting outside of the lock, there is another thread that may be doing it -- within the lock -- and then you have problems.)


> 
> One possible fix off the top of my head would be replacing the last return by a call to some method in the IconDatabaseClient that will get the Image and will return the platform specific image. Does it sound good?

 From my comments above " Remove the call to IconDatabase ::synchronousIconForPageURL and add a new method to converts to your platform specific representation while holding the lock. Then call this method instead."

I think we are saying the same thing.
Comment 12 Brady Eidson 2012-02-16 10:10:44 PST
(In reply to comment #10)
> (In reply to comment #9)
> > > And I also hit this bug in Gtk port when implementing some unit tests for our icon database client
> > 
> > It indicates that there is likely a problem in the code (maybe for multiple platforms), and it needs a volunteer to fix it.
> 
> Well, unless I'm missing something it's pretty clear to me that IconDatabase ::synchronousIconForPageURL cannot be used by any platform as it requires a lock in the mutex used to verify refcounting. This issue was likely never addressed as 1- it does not obviously affect release builds and 2- the code is kind of correct, because even if you don't get the lock, nothing wrong will happen as long as you call it from the same thread that created the image data.

Mac uses it without asserting, and your suggestions as to why it might okay (nothing wrong happening, etc etc) are accurate AFAIK
Comment 13 David Levin 2012-02-16 11:03:21 PST
(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > > And I also hit this bug in Gtk port when implementing some unit tests for our icon database client
> > > 
> > > It indicates that there is likely a problem in the code (maybe for multiple platforms), and it needs a volunteer to fix it.
> > 
> > Well, unless I'm missing something it's pretty clear to me that IconDatabase ::synchronousIconForPageURL cannot be used by any platform as it requires a lock in the mutex used to verify refcounting. This issue was likely never addressed as 1- it does not obviously affect release builds and 2- the code is kind of correct, because even if you don't get the lock, nothing wrong will happen as long as you call it from the same thread that created the image data.
> 
> Mac uses it without asserting, and your suggestions as to why it might okay (nothing wrong happening, etc etc) are accurate AFAIK

I think Mac actually does the right thing to my memory (because it carefully avoid ref counting) but the api as opposed very easily leads to bad behavior.
Comment 14 Sergio Villar Senin 2012-03-30 05:10:34 PDT
Created attachment 134786 [details]
Patch
Comment 15 Early Warning System Bot 2012-03-30 05:23:41 PDT
Comment on attachment 134786 [details]
Patch

Attachment 134786 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12266265
Comment 16 Early Warning System Bot 2012-03-30 05:26:20 PDT
Comment on attachment 134786 [details]
Patch

Attachment 134786 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12275012
Comment 17 Build Bot 2012-03-30 05:34:23 PDT
Comment on attachment 134786 [details]
Patch

Attachment 134786 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12266272
Comment 18 Sergio Villar Senin 2012-03-30 09:07:18 PDT
Created attachment 134822 [details]
Patch
Comment 19 Build Bot 2012-03-30 09:26:02 PDT
Comment on attachment 134822 [details]
Patch

Attachment 134822 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12267284
Comment 20 Eric Seidel (no email) 2012-04-19 16:30:35 PDT
Comment on attachment 134822 [details]
Patch

The reviewers seem to be suggesting that EFL need to change their use of the original function to be threadsafe.  It's not clear to me why adding this extra accessor is correct.
Comment 21 David Levin 2012-04-19 16:39:01 PDT
Actually I think this may the right fix.

The problem was that ref counting was happening outside of the lock and now this access function takes the appropriate lock.

Previously when I looked at this recent patch I though it needed a platform specific reviewer but now I think I may be able to do it.

It needs the window build issue fixed though so I'll leave it as r- so another patch may be done to fix that.
Comment 22 Sergio Villar Senin 2012-04-20 01:05:42 PDT
(In reply to comment #20)
> (From update of attachment 134822 [details])
> The reviewers seem to be suggesting that EFL need to change their use of the original function to be threadsafe.  It's not clear to me why adding this extra accessor is correct.

Eric the problem is that the code inside this new function does some refcounting that must be done inside a lock. It is not something specific to EFL, all the platforms are potential targets. We have reported the same problem in Gtk and Qt guys said they also suffer it. It seems that the only "safe" ports are Win and the Mac because they prevent refcounting in their port code.

I'll provide a patch soon with the fix for the Win port.
Comment 23 Sergio Villar Senin 2012-04-23 08:29:45 PDT
Created attachment 138353 [details]
Patch

Let's see if this fixes the Win EWS
Comment 24 Build Bot 2012-04-23 08:56:27 PDT
Comment on attachment 138353 [details]
Patch

Attachment 138353 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12470903
Comment 25 Sergio Villar Senin 2012-04-23 10:02:59 PDT
Created attachment 138370 [details]
Patch

Another try (crossing fingers)
Comment 26 Build Bot 2012-04-23 10:28:36 PDT
Comment on attachment 138370 [details]
Patch

Attachment 138370 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12470916
Comment 27 Alejandro G. Castro 2012-05-07 09:10:53 PDT
Comment on attachment 138370 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138370&action=review

> Source/WebKit/gtk/webkit/webkitfavicondatabase.cpp:402
> +    GRefPtr<GdkPixbuf> pixbuf = adoptGRef(cairoImageSurfaceToGdkPixbuf(icon));

You have to update at least this part of the patch, this is not going to work anymore because now we have NativeImageCairo since r115385, you need to call surface function to get the cairo_surface_t out of it.

http://trac.webkit.org/changeset/115385

Any update on this patch?
Comment 28 David Levin 2012-05-07 09:17:55 PDT
(In reply to comment #27)
> http://trac.webkit.org/changeset/115385
> 
> Any update on this patch?

It looks like it can't build on Windows for some reason that has yet to be determined, which makes landing it not possible :(.
Comment 29 Sergio Villar Senin 2012-05-07 09:47:07 PDT
(In reply to comment #28)
> (In reply to comment #27)
> > http://trac.webkit.org/changeset/115385
> > 
> > Any update on this patch?
> 
> It looks like it can't build on Windows for some reason that has yet to be determined, which makes landing it not possible :(.

Yeah I have been trying to do that with aroben without much success. Looks like we'd have to give it another try. I guess that only people with Windows builds can help here...
Comment 30 Sergio Villar Senin 2012-05-11 08:39:17 PDT
Created attachment 141424 [details]
Patch
Comment 31 Build Bot 2012-05-11 09:51:30 PDT
Comment on attachment 141424 [details]
Patch

Attachment 141424 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12666548
Comment 32 Gyuyoung Kim 2012-05-11 10:04:08 PDT
Comment on attachment 141424 [details]
Patch

Attachment 141424 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12674221
Comment 33 David Levin 2012-05-11 10:32:22 PDT
Looking at the error on windows, I would guess that Source/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp needs to 
#include "config.h"
Comment 34 Sergio Villar Senin 2012-05-11 10:39:32 PDT
(In reply to comment #33)
> Looking at the error on windows, I would guess that Source/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp needs to 
> #include "config.h"

Thx for the tip! I did some mistake on the EFL code after updating the patch so I'll try it in the next patch
Comment 35 Sergio Villar Senin 2012-05-11 11:42:49 PDT
Created attachment 141460 [details]
Patch
Comment 36 Build Bot 2012-05-11 12:02:55 PDT
Comment on attachment 141460 [details]
Patch

Attachment 141460 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12675285
Comment 37 Yong Li 2012-05-24 14:56:34 PDT
why unconfirmed? changed to new
Comment 38 Yong Li 2012-05-24 14:57:16 PDT
*** Bug 85473 has been marked as a duplicate of this bug. ***
Comment 39 Brady Eidson 2012-05-24 15:43:38 PDT
(In reply to comment #37)
> why unconfirmed? changed to new

I'm guessing because unconfirmed is the default and nobody had bothered to change it yet.
Comment 40 Misha 2012-06-01 14:43:04 PDT
Created attachment 145380 [details]
addition to the proposed patch

It looks like webkit2 Qt port needs this additional code to avoid similar crash.
Comment 41 Allan Sandfeld Jensen 2012-06-04 06:07:39 PDT
There are two alternative solutions to this.

The first is to let SharedBuffer inherit from ThreadSafeRefCounted instead of RefCounted . For all platforms with atomic integers this comes completely free, but for other it will be a lot more expensive.

The second is to copy the sharedbuffer before reusing it the BMPDecoder in the ICODecoder.

Both have been tested and works, but have different performance downsides.
Comment 42 Yong Li 2012-06-04 06:47:48 PDT
(In reply to comment #41)
> There are two alternative solutions to this.
> 
> The first is to let SharedBuffer inherit from ThreadSafeRefCounted instead of RefCounted . For all platforms with atomic integers this comes completely free, but for other it will be a lot more expensive.
> 
> The second is to copy the sharedbuffer before reusing it the BMPDecoder in the ICODecoder.
> 
> Both have been tested and works, but have different performance downsides.

The client just needs the image but not SharedBuffer. Sergio's patch looks good to me.
Comment 43 Allan Sandfeld Jensen 2012-06-04 06:56:44 PDT
(In reply to comment #42)
> (In reply to comment #41)
> > There are two alternative solutions to this.
> > 
> > The first is to let SharedBuffer inherit from ThreadSafeRefCounted instead of RefCounted . For all platforms with atomic integers this comes completely free, but for other it will be a lot more expensive.
> > 
> > The second is to copy the sharedbuffer before reusing it the BMPDecoder in the ICODecoder.
> > 
> > Both have been tested and works, but have different performance downsides.
> 
> The client just needs the image but not SharedBuffer. Sergio's patch looks good to me.

True, the patch actually looks good. I thought there might have been a problem since the patch stalled.
Comment 44 David Levin 2012-06-04 07:46:32 PDT
(In reply to comment #43)
> (In reply to comment #42)
> > (In reply to comment #41)
> > > There are two alternative solutions to this.
> > > 
> > > The first is to let SharedBuffer inherit from ThreadSafeRefCounted instead of RefCounted . For all platforms with atomic integers this comes completely free, but for other it will be a lot more expensive.
> > > 
> > > The second is to copy the sharedbuffer before reusing it the BMPDecoder in the ICODecoder.
> > > 
> > > Both have been tested and works, but have different performance downsides.
> > 
> > The client just needs the image but not SharedBuffer. Sergio's patch looks good to me.
> 
> True, the patch actually looks good. I thought there might have been a problem since the patch stalled.

It breaks the windows build. If that were solved, this could moved forward.
Comment 45 Sergio Villar Senin 2012-06-05 03:00:59 PDT
(In reply to comment #43)
> (In reply to comment #42)
> > (In reply to comment #41)
> > > Both have been tested and works, but have different performance downsides.
> > 
> > The client just needs the image but not SharedBuffer. Sergio's patch looks good to me.
> 
> True, the patch actually looks good. I thought there might have been a problem since the patch stalled.

As some others said I tried several approaches to fix the windows build but I'm a bit stalled now (specially because I don't have a windows machine to test). I spent some hours with aroben a couple of weeks ago but we weren't able to fix it. I suspect that the problem is a hack in the windows build in this file Source/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp where I found this comment:

// FIXME: On Windows, we require all WebKit source files to include config.h
// before including any other files. Failing to include config.h will leave
// WTF_USE_CF undefined, causing build failures in this 
// file. But Mac doesn't have a config.h for WebKit, so we can't include the 
// Windows one here. For now we can just define WTF_USE_CF and
// WTF_USE_CFNETWORK manually, but we need a better long-term solution.
#ifndef WTF_USE_CF
#define WTF_USE_CF 1
#endif

Rings any bell for any Windows hacker?
Comment 46 Allan Sandfeld Jensen 2012-06-05 03:07:35 PDT
(In reply to comment #45)
> #ifndef WTF_USE_CF
> #define WTF_USE_CF 1
> #endif
> 
> Rings any bell for any Windows hacker?

Have you tried git blame / svn blame to see who wrote it?
Comment 47 David Levin 2012-06-05 10:27:49 PDT
(In reply to comment #45)
> (In reply to comment #43)
> > (In reply to comment #42)
> > > (In reply to comment #41)
> > > > Both have been tested and works, but have different performance downsides.
> > > 
> > > The client just needs the image but not SharedBuffer. Sergio's patch looks good to me.
> > 
> > True, the patch actually looks good. I thought there might have been a problem since the patch stalled.
> 
> As some others said I tried several approaches to fix the windows build but I'm a bit stalled now (specially because I don't have a windows machine to test). I spent some hours with aroben a couple of weeks ago but we weren't able to fix it. I suspect that the problem is a hack in the windows build in this file Source/WebKit/cf/WebCoreSupport/WebInspectorClientCF.cpp where I found this comment:

fwiw, the lastest error on Windows was different:
"5>..\..\cf\WebCoreSupport\WebInspectorClientCF.cpp(36) : warning C4067: unexpected tokens following preprocessor directive - expected a newline"

so perhaps the change to WebInspectorClientCF.cpp wasn't done in a way that Windows likes?
(It looks ok in the diff but perhaps these lines are different from the other lines in the file?)
Comment 48 Yong Li 2012-06-15 12:25:51 PDT
Is anyone still working on this?
Comment 49 David Levin 2012-06-15 12:27:08 PDT
(In reply to comment #48)
> Is anyone still working on this?

My impression is that it is stuck due to not figuring out the Windows build issue. If you'd like to take it on, I'm sure folks would appreciate it.
Comment 50 David Levin 2012-06-15 12:27:41 PDT
(In reply to comment #49)
> (In reply to comment #48)
> > Is anyone still working on this?
> 
> My impression is that it is stuck due to not figuring out the Windows build issue. If you'd like to take it on, I'm sure folks would appreciate it.

I think the current patch seems like it is a good fix except for the Windows build break.
Comment 51 Yong Li 2012-06-15 12:35:35 PDT
Created attachment 147876 [details]
Merged together and try to fix windows build

I don't have a Win Safari build environment. I'll use challenge the bot.

I think the build failure is due to missing definition for PLATFORM() and OS() macros.
Comment 52 Yong Li 2012-06-15 13:01:58 PDT
Created attachment 147883 [details]
Try again.

I knew it...

BTW, Sergio, any reason why we want WTF_USE_CG in this file?
Comment 53 Build Bot 2012-06-15 13:41:12 PDT
Comment on attachment 147883 [details]
Try again.

Attachment 147883 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12969088
Comment 54 Build Bot 2012-06-15 13:58:13 PDT
Comment on attachment 147883 [details]
Try again.

Attachment 147883 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12960676
Comment 55 Yong Li 2012-06-18 07:37:48 PDT
Created attachment 148097 [details]
Try fixing the build error (also removed change in webkit/cf)

cannot see why we need the WTF_USE_CG hack in webkit/cf
Comment 56 Build Bot 2012-06-18 08:10:19 PDT
Comment on attachment 148097 [details]
Try fixing the build error (also removed change in webkit/cf)

Attachment 148097 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12979170
Comment 57 Yong Li 2012-06-18 08:42:19 PDT
Created attachment 148104 [details]
put WTF_USE_CG back, this should build now
Comment 58 Yong Li 2012-06-18 09:22:56 PDT
It builds now. Ping reviewers.
Comment 59 David Levin 2012-06-18 23:27:50 PDT
Comment on attachment 148104 [details]
put WTF_USE_CG back, this should build now

View in context: https://bugs.webkit.org/attachment.cgi?id=148104&action=review

This should do it.

Congrats to all! :)

> Source/WebKit/blackberry/WebCoreSupport/FrameLoaderClientBlackBerry.cpp:-1159
> -    if (!img || !img->data())

!img->data() got dropped but that seems ok.

> Source/WebKit/efl/ewk/ewk_history.cpp:341
>          ERR("icon is NULL.");

This error got a little bit broader.

Before if icon->nativeImageForCurrentFrame(); returned 0 this error wasn't printed.

But I think this is ok (in all instances).

> Source/WebKit/efl/ewk/ewk_history.cpp:353
>          ERR("icon is NULL.");

Same comment as above.

> Source/WebKit/efl/ewk/ewk_settings.cpp:218
>          ERR("no icon for url %s", url);

Same comment as before.

> Source/WebKit/efl/ewk/ewk_settings.cpp:232
>          ERR("no icon for url %s", url);

Same comment as before.
Comment 60 Sergio Villar Senin 2012-06-19 01:55:26 PDT
Committed r120694: <http://trac.webkit.org/changeset/120694>