Bug 24624

Summary: Crash in imageLoadEventTimerFired after adoptNode used on <img>, seen with inspector, which uses adoptNode
Product: WebKit Reporter: Darin Adler <darin>
Component: ImagesAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
patch
none
patch, this time with a regression test ap: review+

Darin Adler
Reported 2009-03-16 10:19:10 PDT
1) open attached http://help.apple.com/iphone/guide/desktop.html in safari 2) show web inspector 3) enable JS debugger 4) reload one or more times Crash happens in imageLoadEventTimerFired.
Attachments
patch (20.45 KB, patch)
2009-03-16 10:32 PDT, Darin Adler 		no flags
DetailsFormatted DiffDiff
patch, this time with a regression test (24.12 KB, patch)
2009-03-16 12:54 PDT, Darin Adler 		ap: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2009-03-16 10:19:43 PDT
<rdar://problem/6422850>
Darin Adler
Comment 2 2009-03-16 10:32:45 PDT
Created attachment 28652 [details] patch
Darin Adler
Comment 3 2009-03-16 12:54:26 PDT
Created attachment 28656 [details] patch, this time with a regression test
Alexey Proskuryakov
Comment 4 2009-03-17 01:46:45 PDT
Comment on attachment 28656 [details] patch, this time with a regression test + member functions. Got rid of pointless boolean bit fields that don't make the + object any smaller, since 3 bits and 3 bytes both cost 4 bytes. On PowerPC, bool is still 4 bytes - I don't think that we should regress PPC, especially when that's easy to avoid. + m_dispatchingList = m_dispatchSoonList; + m_dispatchSoonList.clear(); A swap would be more efficient than copying. Index: LayoutTests/fast/dom/HTMLImageElement/resources/image-load-subframe.html =================================================================== --- LayoutTests/fast/dom/HTMLImageElement/resources/image-load-subframe.html (revision 0) +++ LayoutTests/fast/dom/HTMLImageElement/resources/image-load-subframe.html (revision 0) @@ -0,0 +1 @@ +<body><img src="blue_rect.jpg"></body> With a data: URL, the test should be easier to read. It isn't great that a failure in this test is detected as a crash in another one. r=me
Darin Adler
Comment 5 2009-03-17 08:58:42 PDT
(In reply to comment #4) > On PowerPC, bool is still 4 bytes - I don't think that we should regress PPC, > especially when that's easy to avoid. Really? I didn't know that. I had asked about this earlier but I guess it was in person, not on an email list. > + m_dispatchingList = m_dispatchSoonList; > + m_dispatchSoonList.clear(); > > A swap would be more efficient than copying. Sure, I'll do that. > With a data: URL, the test should be easier to read. It isn't great that a > failure in this test is detected as a crash in another one. I was unable to make the test that crashed with the old code with a data URL. I tried for days. I was unable to make a test that would crash within the test. Adding code to wait for the image to load (and hence to wait for the crash) made the bug go away. I'd be happy to improve the test, but I was unable to.
Darin Adler
Comment 6 2009-03-17 09:47:40 PDT
(In reply to comment #5) > I was unable to make the test that crashed with the old code with a data URL. The difficulty was primarily because the data URL didn't have permission to load an image that wasn't also a data URL. Maybe someone else can figure out the trick.
Alexey Proskuryakov
Comment 8 2009-03-17 10:10:19 PDT
(In reply to comment #5) > Really? I didn't know that. I had asked about this earlier but I guess it was > in person, not on an email list. I made a test program that printed sizeof(bool), and it was 4 bytes (12 bytes for a struct with three of those). But perhaps WebKit uses some build setting that changes the default? (In reply to comment #6) > The difficulty was primarily because the data URL didn't have permission to > load an image that wasn't also a data URL. Maybe someone else can figure out > the trick. One possible trick is to use javascript:'' URLs, which share security origin with the opener.
Darin Adler
Comment 9 2009-03-17 10:24:12 PDT
(In reply to comment #8) > > The difficulty was primarily because the data URL didn't have permission to > > load an image that wasn't also a data URL. Maybe someone else can figure out > > the trick. > > One possible trick is to use javascript:'' URLs, which share security origin > with the opener. I'm not going to spend more time on this now, but I'll keep that in mind next time I need to test something like this or if I decided to come back and refine this test.
Note You need to log in before you can comment on or make changes to this bug.