Bug 150609

Summary: Use RunLoopTimer in DataURLDecoder to avoid issues related to runloops
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Page LoadingAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, buildbot, commit-queue, japhet, koivisto, ossy, rniwa, sabouhallawa, webkit-bug-importer
Priority: P1 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 150661    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-mavericks
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2
none
Archive of layout-test-results from ews114 for mac-yosemite
none
Patch
none
Patch none

Description Chris Dumez 2015-10-27 15:36:54 PDT
Use RunLoopTimer in DataURLDecoder to avoid issues related to runloops
Comment 1 Chris Dumez 2015-10-27 15:37:39 PDT
rdar://problem/22702894
Comment 2 Chris Dumez 2015-10-27 16:32:25 PDT
Created attachment 264172 [details]
Patch
Comment 3 WebKit Commit Bot 2015-10-27 16:35:29 PDT
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.
Comment 4 Chris Dumez 2015-10-27 16:35:40 PDT
Created attachment 264173 [details]
Patch
Comment 5 WebKit Commit Bot 2015-10-27 16:38:32 PDT
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 6 Build Bot 2015-10-27 17:19:20 PDT
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
Comment 7 Build Bot 2015-10-27 17:19:25 PDT
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 8 Build Bot 2015-10-27 17:22:56 PDT
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
Comment 9 Build Bot 2015-10-27 17:22:59 PDT
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 10 Build Bot 2015-10-27 17:29:40 PDT
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
Comment 11 Build Bot 2015-10-27 17:29:44 PDT
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
Comment 12 Chris Dumez 2015-10-27 20:25:11 PDT
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 13 Chris Dumez 2015-10-27 20:47:18 PDT
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 14 Chris Dumez 2015-10-27 21:23:16 PDT
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 15 Chris Dumez 2015-10-27 21:25:49 PDT
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.
Comment 16 Chris Dumez 2015-10-27 22:00:11 PDT
Created attachment 264193 [details]
Patch
Comment 17 WebKit Commit Bot 2015-10-27 22:01:44 PDT
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 18 Antti Koivisto 2015-10-28 06:48:18 PDT
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.
Comment 19 Antti Koivisto 2015-10-28 06:49:50 PDT
Anders should take a look too.
Comment 20 Chris Dumez 2015-10-28 09:50:28 PDT
Created attachment 264218 [details]
Patch
Comment 21 Chris Dumez 2015-10-28 09:51:14 PDT
(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 22 WebKit Commit Bot 2015-10-28 10:35:58 PDT
Comment on attachment 264218 [details]
Patch

Clearing flags on attachment: 264218

Committed r191673: <http://trac.webkit.org/changeset/191673>
Comment 23 WebKit Commit Bot 2015-10-28 10:36:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Csaba Osztrogonác 2015-10-28 22:10:25 PDT
(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.
Comment 25 Chris Dumez 2015-10-28 22:12:00 PDT
(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.