Full Text Bug Listing

Huajun.Li

Reported 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)

David Levin

Comment 1 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.

Darin Adler

Comment 2 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?

David Levin

Comment 3 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..

David Levin

Comment 4 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.

Raphael Kubo da Costa (:rakuco)

Comment 5 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?

David Levin

Comment 6 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.

Huajun.Li

Comment 7 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. :)

Sergio Villar Senin

Comment 8 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

David Levin

Comment 9 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.

Sergio Villar Senin

Comment 10 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?

David Levin

Comment 11 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.

Brady Eidson

Comment 12 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

David Levin

Comment 13 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.

Sergio Villar Senin

Comment 14 2012-03-30 05:10:34 PDT

Created attachment 134786 [details] Patch

Early Warning System Bot

Comment 15 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

Early Warning System Bot

Comment 16 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

Build Bot

Comment 17 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

Sergio Villar Senin

Comment 18 2012-03-30 09:07:18 PDT

Created attachment 134822 [details] Patch

Build Bot

Comment 19 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

Eric Seidel (no email)

Comment 20 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.

David Levin

Comment 21 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.

Sergio Villar Senin

Comment 22 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.

Sergio Villar Senin

Comment 23 2012-04-23 08:29:45 PDT

Created attachment 138353 [details] Patch Let's see if this fixes the Win EWS

Build Bot

Comment 24 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

Sergio Villar Senin

Comment 25 2012-04-23 10:02:59 PDT

Created attachment 138370 [details] Patch Another try (crossing fingers)

Build Bot

Comment 26 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

Alejandro G. Castro

Comment 27 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?

David Levin

Comment 28 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 :(.

Sergio Villar Senin

Comment 29 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...

Sergio Villar Senin

Comment 30 2012-05-11 08:39:17 PDT

Created attachment 141424 [details] Patch

Build Bot

Comment 31 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

Gyuyoung Kim

Comment 32 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

David Levin

Comment 33 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"

Sergio Villar Senin

Comment 34 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

Sergio Villar Senin

Comment 35 2012-05-11 11:42:49 PDT

Created attachment 141460 [details] Patch

Build Bot

Comment 36 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

Yong Li

Comment 37 2012-05-24 14:56:34 PDT

why unconfirmed? changed to new

Yong Li

Comment 38 2012-05-24 14:57:16 PDT

*** Bug 85473 has been marked as a duplicate of this bug. ***

Brady Eidson

Comment 39 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.

Misha

Comment 40 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.

Allan Sandfeld Jensen

Comment 41 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.

Yong Li

Comment 42 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.

Allan Sandfeld Jensen

Comment 43 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.

David Levin

Comment 44 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.

Sergio Villar Senin

Comment 45 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?

Allan Sandfeld Jensen

Comment 46 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?

David Levin

Comment 47 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?)

Yong Li

Comment 48 2012-06-15 12:25:51 PDT

Is anyone still working on this?

David Levin

Comment 49 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.

David Levin

Comment 50 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.

Yong Li

Comment 51 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.

Yong Li

Comment 52 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?

Build Bot

Comment 53 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

Build Bot

Comment 54 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

Yong Li

Comment 55 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

Build Bot

Comment 56 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

Yong Li

Comment 57 2012-06-18 08:42:19 PDT

Created attachment 148104 [details] put WTF_USE_CG back, this should build now

Yong Li

Comment 58 2012-06-18 09:22:56 PDT

It builds now. Ping reviewers.

David Levin

Comment 59 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.

Sergio Villar Senin

Comment 60 2012-06-19 01:55:26 PDT

Committed r120694: <http://trac.webkit.org/changeset/120694>

Attachments
Patch (14.97 KB, patch) 2012-03-30 05:10 PDT, Sergio Villar Senin	no flags	Details Formatted Diff Diff
Patch (15.09 KB, patch) 2012-03-30 09:07 PDT, Sergio Villar Senin	no flags	Details Formatted Diff Diff
Patch (15.20 KB, patch) 2012-04-23 08:29 PDT, Sergio Villar Senin	no flags	Details Formatted Diff Diff
Patch (15.06 KB, patch) 2012-04-23 10:02 PDT, Sergio Villar Senin	no flags	Details Formatted Diff Diff
Patch (15.97 KB, patch) 2012-05-11 08:39 PDT, Sergio Villar Senin	no flags	Details Formatted Diff Diff
Patch (17.34 KB, patch) 2012-05-11 11:42 PDT, Sergio Villar Senin	buildbot: commit-queue-	Details Formatted Diff Diff
addition to the proposed patch (2.43 KB, patch) 2012-06-01 14:43 PDT, Misha	no flags	Details Formatted Diff Diff
Merged together and try to fix windows build (19.80 KB, patch) 2012-06-15 12:35 PDT, Yong Li	no flags	Details Formatted Diff Diff
Try again. (19.81 KB, patch) 2012-06-15 13:01 PDT, Yong Li	buildbot: commit-queue-	Details Formatted Diff Diff
Try fixing the build error (also removed change in webkit/cf) (18.48 KB, patch) 2012-06-18 07:37 PDT, Yong Li	buildbot: commit-queue-	Details Formatted Diff Diff
put WTF_USE_CG back, this should build now (19.84 KB, patch) 2012-06-18 08:42 PDT, Yong Li	levin: review+	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Bug 67582