Summary: | Use RunLoopTimer in DataURLDecoder to avoid issues related to runloops | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||
Component: | Page Loading | Assignee: | 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
Chris Dumez
2015-10-27 15:36:54 PDT
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. |