Bug 48014 – Using the decidePolicyForMIMEType delegate message in an asynchronous manner does not work

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Anders Carlsson]

Using the decidePolicyForMIMEType delegate message in an asynchronous manner does not work

[Anders Carlsson]

<rdar://problem/8202716>

[Anders Carlsson]

Created attachment 71348 [details]
Patch

[Adam Barth]

Comment on attachment 71348 [details]
Patch

Can we write a test for this change?

[Anders Carlsson]

Created attachment 71355 [details]
Patch

[Darin Adler]

Comment on attachment 71355 [details]
Patch

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

> WebCore/loader/MainResourceLoader.cpp:235
> +    setDefersLoading(false);

I think this needs a comment indicating where the setDefersLoading(true) is.

> WebCore/loader/MainResourceLoader.cpp:379
> +    // Defer loading while we're waiting for a response.
> +    setDefersLoading(true);

I think this needs a comment indicating where the setDefersLoading(false) is.

Is there a chance that some other code will setDefersLoading(true) and then setDefersLoading(false) somewhere before we get the policy client response? Does this nest properly?

> WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp:371
> -void LayoutTestController::setCustomPolicyDelegate(bool setDelegate, bool permissive)
> +void LayoutTestController::setCustomPolicyDelegate(bool setDelegate, bool permissive, , bool callIgnoreInDecidePolicyForMIMETypeAfterOneSecond)

I don’t think this will compile with that extra comma.

[Adam Barth]

Comment on attachment 71355 [details]
Patch

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

Ok.  These kind of patches are indicative of a larger design problem in the loader...

> WebKitTools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:704
> +			developmentRegion = English;
>  			hasScannedForEncodings = 1;
> +			knownRegions = (
> +				English,
> +				Japanese,
> +				French,
> +				German,
> +			);

Please land without these changes.

> WebKitTools/DumpRenderTree/mac/PolicyDelegate.mm:107
> +    [(NSObject *)listener performSelector:@selector(ignore) withObject:nil afterDelay:1.0];

It's kind of lame that this is hard-coded to 1 second.  I guess its ok.

[Anders Carlsson]

(In reply to comment #5)
> (From update of attachment 71355 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71355&action=review
> 
> > WebCore/loader/MainResourceLoader.cpp:235
> > +    setDefersLoading(false);
> 
> I think this needs a comment indicating where the setDefersLoading(true) is.
> 

Good point.

> > WebCore/loader/MainResourceLoader.cpp:379
> > +    // Defer loading while we're waiting for a response.
> > +    setDefersLoading(true);
> 
> I think this needs a comment indicating where the setDefersLoading(false) is.

Good point.

> 
> Is there a chance that some other code will setDefersLoading(true) and then setDefersLoading(false) somewhere before we get the policy client response? Does this nest properly?

It does not nest properly, but I don't think that code can call setDefersLoading(false) this early on. I'll double check and add a counter.

> 
> > WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp:371
> > -void LayoutTestController::setCustomPolicyDelegate(bool setDelegate, bool permissive)
> > +void LayoutTestController::setCustomPolicyDelegate(bool setDelegate, bool permissive, , bool callIgnoreInDecidePolicyForMIMETypeAfterOneSecond)
> 
> I don’t think this will compile with that extra comma.

Whoops, good catch!

[Anders Carlsson]

Committed r70367: <http://trac.webkit.org/changeset/70367>

[Csaba Osztrogonác]

(In reply to comment #8)
> Committed r70367: <http://trac.webkit.org/changeset/70367>

Reopen, because it was rolled out by http://trac.webkit.org/changeset/70385
( https://bugs.webkit.org/show_bug.cgi?id=48176 )

It made 8-10 tests crash on Qt bot:
http://build.webkit.org/results/Qt%20Linux%20Release/r70367%20%2822455%29/results.html

[Csaba Osztrogonác]

Comment on attachment 71355 [details]
Patch

remove r+ from landed and rolled-out patch

[Csaba Osztrogonác]

It seems crashes caused by buggy Qt 4.6.2 :/ I updated our bot to 
Qt 4.6.3 and tested https://bugs.webkit.org/attachment.cgi?id=71355 .
It works for me without any regression.

Sorry for roll-out and thanks for your help 
in debugging and finding the root of the crashes.

[Anders Carlsson]

We decided not to land this after all, it was handled in http://trac.webkit.org/projects/webkit/changeset/72122 instead.