188033 – [Curl] Crash on synchronous request via ResourceHandle.

Bug 188033 - [Curl] Crash on synchronous request via ResourceHandle.

Summary: [Curl] Crash on synchronous request via ResourceHandle.

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Platform (show other bugs)
Version:	WebKit Nightly Build
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Basuke Suzuki

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2018-07-25 17:44 PDT by Basuke Suzuki
Modified:	2018-07-27 11:21 PDT (History)
CC List:	8 users (show)

See Also:

Attachments
FIX (9.32 KB, patch) 2018-07-25 21:41 PDT, Basuke Suzuki	ews-watchlist: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews100 for mac-sierra (2.52 MB, application/zip) 2018-07-25 22:51 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.27 MB, application/zip) 2018-07-25 22:58 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews115 for mac-sierra (3.85 MB, application/zip) 2018-07-25 23:29 PDT, EWS Watchlist	no flags	Details
PATCH (7.27 KB, patch) 2018-07-26 01:11 PDT, Basuke Suzuki	Hironori.Fujii: review-	Details \| Formatted Diff \| Diff
Fix (3.89 KB, patch) 2018-07-26 08:08 PDT, Basuke Suzuki	no flags	Details \| Formatted Diff \| Diff
Fix (3.94 KB, patch) 2018-07-26 08:19 PDT, Basuke Suzuki	Hironori.Fujii: review- ews-watchlist: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews206 for win-future (12.91 MB, application/zip) 2018-07-26 12:44 PDT, EWS Watchlist	no flags	Details
PATCH (3.00 KB, patch) 2018-07-27 09:31 PDT, Basuke Suzuki	achristensen: review+	Details \| Formatted Diff \| Diff
Patch for landing (3.02 KB, patch) 2018-07-27 10:40 PDT, Basuke Suzuki	no flags	Details \| Formatted Diff \| Diff
Show Obsolete (9) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Basuke Suzuki 2018-07-25 17:44:42 PDT

On WebKitLegacy, it crashes while initializing when it is requested as a synchronous request.

Comment 1 Basuke Suzuki 2018-07-25 21:41:59 PDT

Created attachment 345820 [details]
FIX

Comment 2 EWS Watchlist 2018-07-25 22:51:41 PDT

Comment on attachment 345820 [details]
FIX

Attachment 345820 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8658565

New failing tests:
http/tests/xmlhttprequest/simple-sync.html

Comment 3 EWS Watchlist 2018-07-25 22:51:43 PDT

Created attachment 345826 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

Comment 4 EWS Watchlist 2018-07-25 22:58:14 PDT

Comment on attachment 345820 [details]
FIX

Attachment 345820 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8658585

New failing tests:
http/tests/xmlhttprequest/simple-sync.html

Comment 5 EWS Watchlist 2018-07-25 22:58:16 PDT

Created attachment 345827 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

Comment 6 EWS Watchlist 2018-07-25 23:29:43 PDT

Comment on attachment 345820 [details]
FIX

Attachment 345820 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8658602

New failing tests:
http/tests/xmlhttprequest/simple-sync.html

Comment 7 EWS Watchlist 2018-07-25 23:29:45 PDT

Created attachment 345830 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

Comment 8 Basuke Suzuki 2018-07-26 01:11:54 PDT

Created attachment 345835 [details]
PATCH

Comment 9 Fujii Hironori 2018-07-26 02:41:17 PDT

Comment on attachment 345835 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=345835&action=review

> LayoutTests/platform/wincairo/TestExpectations:863
> +http/tests/xmlhttprequest/simple-sync.html [ Pass ]

You added a new test case simple-sync.html in r234228.
https://trac.webkit.org/changeset/234228/
This is not a test gardening.
Test gardening is just marking test cases in TestExpectations.

In general, if you find a bug, create a test case and fixing the bug in a single patch.
You don't need to split the commits.

> Source/WebCore/ChangeLog:8
> +        The timing of instanciation of delegate was wrong. Move it inside

instanciation → instantiation

> Source/WebCore/ChangeLog:10
> +        it's not null.

It's a bad idea to unify fixing bug and refactoring into a single patch.
Split this patch into two, fixing bug and refactoring.

Comment 10 Fujii Hironori 2018-07-26 02:50:04 PDT

At the first place, ResourceHandle::delegate() should be removed in favor of ResourceHandle::d (which is a ResourceHandleInternal).

Comment 11 Fujii Hironori 2018-07-26 02:56:12 PDT

Comment on attachment 345835 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=345835&action=review

>> LayoutTests/platform/wincairo/TestExpectations:863
>> +http/tests/xmlhttprequest/simple-sync.html [ Pass ]
> 
> You added a new test case simple-sync.html in r234228.
> https://trac.webkit.org/changeset/234228/
> This is not a test gardening.
> Test gardening is just marking test cases in TestExpectations.
> 
> In general, if you find a bug, create a test case and fixing the bug in a single patch.
> You don't need to split the commits.

In general, marking a test case "[ Pass ]" isn't a good idea.

WinCairo port should remote the following line.

> http/tests [ Skip ]

And, add [ Skip ] for the subdirectories.

> http/tests/appcache [ Skip ]
> http/tests/blink [ Skip ]
> ...

Comment 12 Fujii Hironori 2018-07-26 03:08:59 PDT

Comment on attachment 345835 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=345835&action=review

>>> LayoutTests/platform/wincairo/TestExpectations:863
>>> +http/tests/xmlhttprequest/simple-sync.html [ Pass ]
>> 
>> You added a new test case simple-sync.html in r234228.
>> https://trac.webkit.org/changeset/234228/
>> This is not a test gardening.
>> Test gardening is just marking test cases in TestExpectations.
>> 
>> In general, if you find a bug, create a test case and fixing the bug in a single patch.
>> You don't need to split the commits.
> 
> In general, marking a test case "[ Pass ]" isn't a good idea.
> 
> WinCairo port should remote the following line.

Just out of curiosity. You said "This crash prevents DumpRenderTree running for many http tests." in IRC. Why did you create a new test case?

Comment 13 Fujii Hironori 2018-07-26 03:09:54 PDT

Do you know which revision did introduce this bug?

Comment 14 Fujii Hironori 2018-07-26 03:22:31 PDT

(In reply to Fujii Hironori from comment #10)
> At the first place, ResourceHandle::delegate() should be removed in favor of
> ResourceHandle::d (which is a ResourceHandleInternal).

I'm wrong. Mac port also has ResourceHandle::delegate(). Never mind.

Comment 15 Basuke Suzuki 2018-07-26 08:06:51 PDT

(In reply to Fujii Hironori from comment #11)
> Comment on attachment 345835 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345835&action=review
> 
> >> LayoutTests/platform/wincairo/TestExpectations:863
> >> +http/tests/xmlhttprequest/simple-sync.html [ Pass ]
> > 
> > You added a new test case simple-sync.html in r234228.
> > https://trac.webkit.org/changeset/234228/
> > This is not a test gardening.
> > Test gardening is just marking test cases in TestExpectations.
> > 
> > In general, if you find a bug, create a test case and fixing the bug in a single patch.
> > You don't need to split the commits.
> 
> In general, marking a test case "[ Pass ]" isn't a good idea.
> 
> WinCairo port should remote the following line.
> 
> > http/tests [ Skip ]
> 
> And, add [ Skip ] for the subdirectories.
> 
> > http/tests/appcache [ Skip ]
> > http/tests/blink [ Skip ]
> > ...

At this moment,  it's too unpractical to write down every tests directory [ Skip ]. WebSocket is ready to follow that so that I did that. Others are far from that point. I will move http/tests [Skip] line close to this area to make it clear.

Comment 16 Basuke Suzuki 2018-07-26 08:08:04 PDT

Created attachment 345844 [details]
Fix

Remove refactoring part.

Comment 17 Basuke Suzuki 2018-07-26 08:19:45 PDT

Created attachment 345846 [details]
Fix

Comment 18 EWS Watchlist 2018-07-26 12:43:48 PDT

Comment on attachment 345846 [details]
Fix

Attachment 345846 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8664522

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

Comment 19 EWS Watchlist 2018-07-26 12:44:00 PDT

Created attachment 345858 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

Comment 20 Fujii Hironori 2018-07-26 18:34:09 PDT

Comment on attachment 345846 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=345846&action=review

In general, you need to setup testing framework before implementing some features or refactoring to avoid regressions like Bug 188033.

> LayoutTests/platform/wincairo/TestExpectations:850
> +

Don't move this.

> LayoutTests/platform/wincairo/TestExpectations:869
> +http/tests/xmlhttprequest/simple-sync.html [ Pass ]

Remove these two lines if it is too unpractical at the moment.

# XMLHTTPRequest (sync)
http/tests/xmlhttprequest/simple-sync.html [ Crash ]

> LayoutTests/platform/wincairo/TestExpectations:-1574
> -http/tests [ Skip ]

Don't move this.

Comment 21 Basuke Suzuki 2018-07-26 19:28:13 PDT

Comment on attachment 345846 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=345846&action=review

>> LayoutTests/platform/wincairo/TestExpectations:850
>> +
> 
> Don't move this.

Why?

Comment 22 Fujii Hironori 2018-07-26 19:38:38 PDT

(In reply to Basuke Suzuki from comment #21)
> Why?

Don't mix fixing a crash bug and moving marking into a single patch.
You can do gardening by unreviewed commit.

Comment 23 Basuke Suzuki 2018-07-26 19:51:38 PDT

Oh, okay. Got it.

Comment 24 Fujii Hironori 2018-07-26 19:55:52 PDT

On the other hand, removing "http/tests/xmlhttprequest/simple-sync.html [ Crash ]" should be in this patch because this patch fix the crash.

Comment 25 Fujii Hironori 2018-07-27 03:38:41 PDT

Comment on attachment 345846 [details]
Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=345846&action=review

>> LayoutTests/platform/wincairo/TestExpectations:869
>> +http/tests/xmlhttprequest/simple-sync.html [ Pass ]
> 
> Remove these two lines if it is too unpractical at the moment.
> 
> # XMLHTTPRequest (sync)
> http/tests/xmlhttprequest/simple-sync.html [ Crash ]

This test doesn't crash in WinCairo BuildBot, but text diff.
Will this really be Pass by applying this patch?

https://build.webkit.org/results/WinCairo%2064-bit%20WKL%20Release%20(Tests)/r234293%20(762)/http/tests/xmlhttprequest/simple-sync-diff.txt

Comment 26 Basuke Suzuki 2018-07-27 07:20:52 PDT

(In reply to Fujii Hironori from comment #25)
> Comment on attachment 345846 [details]
> Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345846&action=review
> 
> >> LayoutTests/platform/wincairo/TestExpectations:869
> >> +http/tests/xmlhttprequest/simple-sync.html [ Pass ]
> > 
> > Remove these two lines if it is too unpractical at the moment.
> > 
> > # XMLHTTPRequest (sync)
> > http/tests/xmlhttprequest/simple-sync.html [ Crash ]
> 
> This test doesn't crash in WinCairo BuildBot, but text diff.
> Will this really be Pass by applying this patch?
> 
> https://build.webkit.org/results/WinCairo%2064-bit%20WKL%20Release%20(Tests)/
> r234293%20(762)/http/tests/xmlhttprequest/simple-sync-diff.txt

+CONSOLE MESSAGE: line 13: Cross-origin redirection denied by Cross-Origin Resource Sharing policy.
+CONSOLE MESSAGE: line 13: XMLHttpRequest cannot load http://127.0.0.1:8000/xmlhttprequest/resources/print-referer.php due to access control checks.
+CONSOLE MESSAGE: line 13: NetworkError:  A network error occurred.

That message is the next chunk of bug I want to solve. I didn't research where does dis message come from, but once it happens, actual communication doesn't happen so that crash never happen.

Comment 27 Basuke Suzuki 2018-07-27 09:31:07 PDT

Created attachment 345918 [details]
PATCH

Comment 28 Alex Christensen 2018-07-27 09:59:21 PDT

Comment on attachment 345918 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=345918&action=review

> LayoutTests/platform/wincairo/TestExpectations:931
> -http/tests/xmlhttprequest/simple-sync.html [ Crash ]
> +http/tests/xmlhttprequest/simple-sync.html [ Pass ]

Why not just remove this line?

Comment 29 Basuke Suzuki 2018-07-27 10:30:21 PDT

(In reply to Alex Christensen from comment #28)
> Comment on attachment 345918 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345918&action=review
> 
> > LayoutTests/platform/wincairo/TestExpectations:931
> > -http/tests/xmlhttprequest/simple-sync.html [ Crash ]
> > +http/tests/xmlhttprequest/simple-sync.html [ Pass ]
> 
> Why not just remove this line?

There's other issues so that we have to skip xmlhttprequest/ directory. We need a few more fix to open the gate.

Comment 30 Basuke Suzuki 2018-07-27 10:40:53 PDT

Created attachment 345925 [details]
Patch for landing

Alex, thanks for r+

Comment 31 WebKit Commit Bot 2018-07-27 11:19:25 PDT

Comment on attachment 345925 [details]
Patch for landing

Clearing flags on attachment: 345925

Committed r234317: <https://trac.webkit.org/changeset/234317>

Comment 32 WebKit Commit Bot 2018-07-27 11:19:27 PDT

All reviewed patches have been landed.  Closing bug.

Comment 33 Radar WebKit Bug Importer 2018-07-27 11:21:39 PDT

<rdar://problem/42667839>