Bug 150609 – Use RunLoopTimer in DataURLDecoder to avoid issues related to runloops

• Component: Page Loading
• Status: RESOLVED FIXED

[Chris Dumez]

Use RunLoopTimer in DataURLDecoder to avoid issues related to runloops

[Chris Dumez]

rdar://problem/22702894

[Chris Dumez]

Created attachment 264172 [details]
Patch

[WebKit Commit Bot]

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.

[Chris Dumez]

Created attachment 264173 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 264193 [details]
Patch

[WebKit Commit Bot]

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.

[Antti Koivisto]

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.

[Antti Koivisto]

Anders should take a look too.

[Chris Dumez]

Created attachment 264218 [details]
Patch

[Chris Dumez]

(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.

[WebKit Commit Bot]

Comment on attachment 264218 [details]
Patch

Clearing flags on attachment: 264218

Committed r191673: <http://trac.webkit.org/changeset/191673>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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.

[Chris Dumez]

(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.