WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
67582
Calling nativeImageForCurrentFrame() causes assertion failure: m_verifier.isSafeToUse()
https://bugs.webkit.org/show_bug.cgi?id=67582
Summary
Calling nativeImageForCurrentFrame() causes assertion failure: m_verifier.isS...
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)
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.
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
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug