Bug 188033

Created attachment 345820 [details]
FIX

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

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

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

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

Created attachment 345835 [details]
PATCH

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.

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

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 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?

Do you know which revision did introduce this bug?

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

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

Created attachment 345844 [details]
Fix

Remove refactoring part.

Created attachment 345846 [details]
Fix

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

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 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 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?

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

Oh, okay. Got it.

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

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

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

Created attachment 345918 [details]
PATCH

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?

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

Created attachment 345925 [details]
Patch for landing

Alex, thanks for r+

Comment on attachment 345925 [details]
Patch for landing

Clearing flags on attachment: 345925

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

All reviewed patches have been landed.  Closing bug.

<rdar://problem/42667839>