Use RunLoopTimer in DataURLDecoder to avoid issues related to runloops
rdar://problem/22702894
Created attachment 264172 [details] Patch
Attachment 264172 [details] did not pass style-queue: ERROR: Source/WebCore/loader/ResourceLoader.cpp:256: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 264173 [details] Patch
Attachment 264173 [details] did not pass style-queue: ERROR: Source/WebCore/loader/ResourceLoader.cpp:256: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 264173 [details] Patch Attachment 264173 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/346261 New failing tests: svg/as-image/svg-image-with-svg-data-uri.html svg/as-image/svg-image-with-data-uri-reloading.html svg/text/text-default-font-size.html svg/as-image/svg-image-with-data-uri-use-data-uri.svg svg/as-image/svg-image-with-data-uri-from-canvas.html svg/as-image/svg-image-with-data-uri-background.html
Created attachment 264179 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 264173 [details] Patch Attachment 264173 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/346273 New failing tests: svg/as-image/svg-image-with-svg-data-uri.html svg/as-image/svg-image-with-data-uri-reloading.html svg/as-image/svg-image-with-data-uri-background.html svg/as-image/svg-image-with-data-uri-use-data-uri.svg svg/as-image/svg-image-with-data-uri-from-canvas.html svg/text/text-default-font-size.html
Created attachment 264180 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 264173 [details] Patch Attachment 264173 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/346286 New failing tests: svg/as-image/svg-image-with-svg-data-uri.html svg/as-image/svg-image-with-data-uri-reloading.html svg/text/text-default-font-size.html svg/as-image/svg-image-with-data-uri-use-data-uri.svg svg/as-image/svg-image-with-data-uri-from-canvas.html svg/as-image/svg-image-with-data-uri-background.html
Created attachment 264182 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Said, I see that you added most of these SVG tests in <http://trac.webkit.org/changeset/179626>. Any idea what could be wrong with my change? I have no idea these SVG tests are failing at the moment.
Comment on attachment 264173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264173&action=review > Source/WebCore/platform/network/DataURLDecoder.cpp:83 > + void timerFired() timerFired() does not seem to be called for the SVG tests that are failing, even though startTimer() is called.
Comment on attachment 264173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264173&action=review > Source/WebCore/loader/ResourceLoader.cpp:254 > + m_frame->page()->scheduledRunLoopPairs(), The issue seems to be that this returns nullptr in the SVG test cases. It is supposed to be initialized in WebPage::platformInitialize(): m_page->addSchedulePair(SchedulePair::create([NSRunLoop currentRunLoop], kCFRunLoopCommonModes)); WebPage::platformInitialize() gets called but not for this particular page for some reason.
Comment on attachment 264173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264173&action=review >> Source/WebCore/loader/ResourceLoader.cpp:254 >> + m_frame->page()->scheduledRunLoopPairs(), > > The issue seems to be that this returns nullptr in the SVG test cases. It is supposed to be initialized in WebPage::platformInitialize(): > m_page->addSchedulePair(SchedulePair::create([NSRunLoop currentRunLoop], kCFRunLoopCommonModes)); > > WebPage::platformInitialize() gets called but not for this particular page for some reason. bool SVGImage::dataChanged(bool allDataReceived) creates its own Page :( But we don't call addSchedulePair() for that page.
Created attachment 264193 [details] Patch
Attachment 264193 [details] did not pass style-queue: ERROR: Source/WebCore/loader/ResourceLoader.cpp:256: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 264193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264193&action=review r=me since it fixes a bug but I think it could be cleaner. > Source/WebCore/platform/network/DataURLDecoder.h:57 > +#if HAVE(RUNLOOP_TIMER) > +void decode(const URL&, SchedulePairHashSet*, DecodeCompletionHandler); > +#else > void decode(const URL&, DecodeCompletionHandler); > +#endif This leads to many ugly ifdefs. I think you could do this much more cleanly along these lines: struct ScheduleContext { #if HAVE(RUNLOOP_TIMER) SchedulePairHashSet scheduledPairs; #endif }; void decode(const URL&, ScheduleContext&, DecodeCompletionHandler); and passing ScheduleContext around in all cases.
Anders should take a look too.
Created attachment 264218 [details] Patch
(In reply to comment #18) > Comment on attachment 264193 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264193&action=review > > r=me since it fixes a bug but I think it could be cleaner. > > > Source/WebCore/platform/network/DataURLDecoder.h:57 > > +#if HAVE(RUNLOOP_TIMER) > > +void decode(const URL&, SchedulePairHashSet*, DecodeCompletionHandler); > > +#else > > void decode(const URL&, DecodeCompletionHandler); > > +#endif > > This leads to many ugly ifdefs. I think you could do this much more cleanly > along these lines: > > struct ScheduleContext { > #if HAVE(RUNLOOP_TIMER) > SchedulePairHashSet scheduledPairs; > #endif > }; > > void decode(const URL&, ScheduleContext&, DecodeCompletionHandler); > > and passing ScheduleContext around in all cases. Good idea, I did this in the latest iteration.
Comment on attachment 264218 [details] Patch Clearing flags on attachment: 264218 Committed r191673: <http://trac.webkit.org/changeset/191673>
All reviewed patches have been landed. Closing bug.
(In reply to comment #22) > Comment on attachment 264218 [details] > Patch > > Clearing flags on attachment: 264218 > > Committed r191673: <http://trac.webkit.org/changeset/191673> It made 50+ tests timeout and bots early exit. Check EFL, GTK, Apple Win, iOS bots for details. Please fix this serious regression ASAP.
(In reply to comment #24) > (In reply to comment #22) > > Comment on attachment 264218 [details] > > Patch > > > > Clearing flags on attachment: 264218 > > > > Committed r191673: <http://trac.webkit.org/changeset/191673> > > It made 50+ tests timeout and bots early exit. > Check EFL, GTK, Apple Win, iOS bots for details. > > Please fix this serious regression ASAP. Yes, I am about to upload a patch to Bug 150661.