WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
150609
Use RunLoopTimer in DataURLDecoder to avoid issues related to runloops
https://bugs.webkit.org/show_bug.cgi?id=150609
Summary
Use RunLoopTimer in DataURLDecoder to avoid issues related to runloops
Chris Dumez
Reported
2015-10-27 15:36:54 PDT
Use RunLoopTimer in DataURLDecoder to avoid issues related to runloops
Attachments
Patch
(7.36 KB, patch)
2015-10-27 16:32 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(7.28 KB, patch)
2015-10-27 16:35 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews103 for mac-mavericks
(816.75 KB, application/zip)
2015-10-27 17:19 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2
(898.64 KB, application/zip)
2015-10-27 17:22 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from ews114 for mac-yosemite
(903.36 KB, application/zip)
2015-10-27 17:29 PDT
,
Build Bot
no flags
Details
Patch
(23.76 KB, patch)
2015-10-27 22:00 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Patch
(23.79 KB, patch)
2015-10-28 09:50 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
2015-10-27 15:37:39 PDT
rdar://problem/22702894
Chris Dumez
Comment 2
2015-10-27 16:32:25 PDT
Created
attachment 264172
[details]
Patch
WebKit Commit Bot
Comment 3
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.
Chris Dumez
Comment 4
2015-10-27 16:35:40 PDT
Created
attachment 264173
[details]
Patch
WebKit Commit Bot
Comment 5
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.
Build Bot
Comment 6
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
Build Bot
Comment 7
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
Build Bot
Comment 8
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
Build Bot
Comment 9
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
Build Bot
Comment 10
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
Build Bot
Comment 11
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
Chris Dumez
Comment 12
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.
Chris Dumez
Comment 13
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.
Chris Dumez
Comment 14
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.
Chris Dumez
Comment 15
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.
Chris Dumez
Comment 16
2015-10-27 22:00:11 PDT
Created
attachment 264193
[details]
Patch
WebKit Commit Bot
Comment 17
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.
Antti Koivisto
Comment 18
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.
Antti Koivisto
Comment 19
2015-10-28 06:49:50 PDT
Anders should take a look too.
Chris Dumez
Comment 20
2015-10-28 09:50:28 PDT
Created
attachment 264218
[details]
Patch
Chris Dumez
Comment 21
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.
WebKit Commit Bot
Comment 22
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
>
WebKit Commit Bot
Comment 23
2015-10-28 10:36:04 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 24
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.
Chris Dumez
Comment 25
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
.
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