Bug 48014 - Using the decidePolicyForMIMEType delegate message in an asynchronous manner does not work
Summary: Using the decidePolicyForMIMEType delegate message in an asynchronous manner ...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Anders Carlsson
URL:
Keywords: InRadar
Depends on: 48176
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-20 15:27 PDT by Anders Carlsson
Modified: 2010-11-17 14:44 PST (History)
2 users (show)

See Also:


Attachments
Patch (1.83 KB, patch)
2010-10-20 15:30 PDT, Anders Carlsson
no flags Details | Formatted Diff | Diff
Patch (18.79 KB, patch)
2010-10-20 16:26 PDT, Anders Carlsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2010-10-20 15:27:46 PDT
Using the decidePolicyForMIMEType delegate message in an asynchronous manner does not work
Comment 1 Anders Carlsson 2010-10-20 15:29:05 PDT
<rdar://problem/8202716>
Comment 2 Anders Carlsson 2010-10-20 15:30:39 PDT
Created attachment 71348 [details]
Patch
Comment 3 Adam Barth 2010-10-20 15:31:57 PDT
Comment on attachment 71348 [details]
Patch

Can we write a test for this change?
Comment 4 Anders Carlsson 2010-10-20 16:26:55 PDT
Created attachment 71355 [details]
Patch
Comment 5 Darin Adler 2010-10-20 16:30:19 PDT
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.
Comment 6 Adam Barth 2010-10-20 16:31:03 PDT
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.
Comment 7 Anders Carlsson 2010-10-21 09:38:35 PDT
(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!
Comment 8 Anders Carlsson 2010-10-22 16:49:18 PDT
Committed r70367: <http://trac.webkit.org/changeset/70367>
Comment 9 Csaba Osztrogonác 2010-10-23 00:45:36 PDT
(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
Comment 10 Csaba Osztrogonác 2010-10-23 00:46:18 PDT
Comment on attachment 71355 [details]
Patch

remove r+ from landed and rolled-out patch
Comment 11 Csaba Osztrogonác 2010-10-26 16:05:55 PDT
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.
Comment 12 Anders Carlsson 2010-11-17 14:44:40 PST
We decided not to land this after all, it was handled in http://trac.webkit.org/projects/webkit/changeset/72122 instead.