Bug 60059 - AssociatedURLLoader reports some errors synchronously
Summary: AssociatedURLLoader reports some errors synchronously
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-03 13:40 PDT by Bill Budge
Modified: 2011-06-16 16:53 PDT (History)
8 users (show)

See Also:


Attachments
Proposed Patch (5.32 KB, patch)
2011-05-03 13:46 PDT, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (5.37 KB, patch)
2011-05-03 13:54 PDT, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (5.43 KB, patch)
2011-05-03 14:29 PDT, Bill Budge
abarth: review-
Details | Formatted Diff | Diff
Proposed Patch (5.48 KB, patch)
2011-06-02 13:00 PDT, Bill Budge
abarth: review-
Details | Formatted Diff | Diff
Proposed Patch (5.42 KB, patch)
2011-06-03 11:14 PDT, Bill Budge
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (5.57 KB, patch)
2011-06-03 11:44 PDT, Bill Budge
abarth: review-
Details | Formatted Diff | Diff
Proposed Patch (5.59 KB, patch)
2011-06-03 13:24 PDT, Bill Budge
abarth: review-
Details | Formatted Diff | Diff
Proposed Patch (17.08 KB, patch)
2011-06-08 17:33 PDT, Bill Budge
no flags Details | Formatted Diff | Diff
Proposed Patch (17.08 KB, patch)
2011-06-08 17:48 PDT, Bill Budge
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (17.11 KB, patch)
2011-06-09 05:40 PDT, Bill Budge
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (17.10 KB, patch)
2011-06-09 09:54 PDT, Bill Budge
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff
Proposed Patch (17.11 KB, patch)
2011-06-09 10:47 PDT, Bill Budge
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bill Budge 2011-05-03 13:40:40 PDT
AssociatedURLLoader uses DocumentThreadableLoader internally, which reports some same-origin violation errors synchronously. Use a Timer to defer those error callbacks until after control has returned to the caller.
Comment 1 Bill Budge 2011-05-03 13:46:16 PDT
Created attachment 92120 [details]
Proposed Patch
Comment 2 Bill Budge 2011-05-03 13:54:30 PDT
Created attachment 92123 [details]
Proposed Patch
Comment 3 Bill Budge 2011-05-03 14:29:53 PDT
Created attachment 92131 [details]
Proposed Patch

WebError.reason is not a reliable test for whether something is a true error, so use an m_didFail flag.
Comment 4 Darin Fisher (:fishd, Google) 2011-05-03 14:41:23 PDT
(In reply to comment #3)
> Created an attachment (id=92131) [details]
> Proposed Patch
> 
> WebError.reason is not a reliable test for whether something is a true error, so use an m_didFail flag.

so you see AssociatedURLLoader::ClientAdapter::didFail being called with a non-error?  that seems indicative of another bug.  surely, didFail should only be reporting errors :)
Comment 5 Bill Budge 2011-05-03 15:20:05 PDT
I didn't make that very clear. The m_error field is initialized with the default constructor and reason is 0. But didFail can get an error that also has reason 0. So it can't reliably decide (in enableErrorNotifications) whether or not didFail got called.
Comment 6 Darin Fisher (:fishd, Google) 2011-05-03 15:28:07 PDT
(In reply to comment #5)
> I didn't make that very clear. The m_error field is initialized with the default constructor and reason is 0. But didFail can get an error that also has reason 0. So it can't reliably decide (in enableErrorNotifications) whether or not didFail got called.

That's what I understood.  It seems buggy for didFail to get an error that has reason == 0.  It should have a non-zero reason value if there was an error.
Comment 7 Bill Budge 2011-05-03 15:35:56 PDT
I can see the ResourceError constructor call in DocumentThreadableLoader::create. I also see you have a TODO to provide more detailed error information in ppb_url_loader_impl.cc in Webkit::plugins. Should I put together a patch to add better error messages in WebCore before we do this one?
Comment 8 Adam Barth 2011-06-02 11:22:51 PDT
Comment on attachment 92131 [details]
Proposed Patch

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

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:185
> +        m_errorTimer = new Timer<ClientAdapter>(this, &ClientAdapter::notifyError);

WebKit frowns upon "naked new", meaning calls to new should immediately be followed by either adoptPtr or adoptRef, as appropriate.  In any case, we just normally make these Timer objects be regular members, not pointers.  Actually, I'm surprised this patch even compiles.  There must be something wrong somwhere.
Comment 9 Bill Budge 2011-06-02 11:35:29 PDT
Hi Adam, thanks for looking at this. I'm planning on adding error codes to the ResourceErrors returned from WebCore, so we can return better error codes. Then I'll return to this patch and fix the problem you found. What do you think?
Comment 10 Adam Barth 2011-06-02 11:51:27 PDT
> What do you think?

Sure.  I was just going through my inbox and took a look at the patch.
Comment 11 Bill Budge 2011-06-02 13:00:03 PDT
Created attachment 95791 [details]
Proposed Patch

Adam, on second thought, we need this to fix some things in Chrome and I can add the error codes later.
Comment 12 Bill Budge 2011-06-03 10:43:43 PDT
For this to compile, LOOSE_OWN_PTR must be defined. I used adoptPtr, since the constructor for timer needs 'this'.
Comment 13 Adam Barth 2011-06-03 10:45:45 PDT
Comment on attachment 95791 [details]
Proposed Patch

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

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:89
> +    OwnPtr<Timer<ClientAdapter> > m_errorTimer;

This should just be a regular member, not an OwnPtr.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:190
> +void AssociatedURLLoader::ClientAdapter::notifyError(Timer<ClientAdapter>* timer)

You should assert that |timer| has the value you expect.
Comment 14 Bill Budge 2011-06-03 11:14:58 PDT
Created attachment 95931 [details]
Proposed Patch

Made m_errorTimer a direct member.
Comment 15 WebKit Review Bot 2011-06-03 11:17:34 PDT
Attachment 95931 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/src/AssociatedURLLoader.cpp:187:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 WebKit Review Bot 2011-06-03 11:21:45 PDT
Comment on attachment 95931 [details]
Proposed Patch

Attachment 95931 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8765128
Comment 17 Adam Barth 2011-06-03 11:23:33 PDT
(In reply to comment #12)
> For this to compile, LOOSE_OWN_PTR must be defined.

You're not allowed to define LOOSE_OWN_PTR.  That's code we're deleting.
Comment 18 Bill Budge 2011-06-03 11:44:42 PDT
Created attachment 95934 [details]
Proposed Patch
Comment 19 Adam Barth 2011-06-03 11:55:04 PDT
Comment on attachment 95934 [details]
Proposed Patch

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

This is starting to look good, but we're still lacking a test.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:190
> +{

Please add the ASSERT I asked for in the previous review.
Comment 20 Bill Budge 2011-06-03 13:24:37 PDT
Created attachment 95955 [details]
Proposed Patch

Added ASSERT in notifyError.

I can't find any obvious tests of AssociatedURLLoader specifically. Also, I'm having trouble running LayoutTests (DumpRenderTree build is failing on command line).
Comment 21 Adam Barth 2011-06-03 13:26:46 PDT
Comment on attachment 95955 [details]
Proposed Patch

Thanks for the ASSERT, but we need tests for each change.  We certainly don't want entire files with zero test coverage.
Comment 22 Bill Budge 2011-06-08 17:33:28 PDT
Created attachment 96514 [details]
Proposed Patch

Added unit tests for successful loading (same- and cross-origin) and asynchronous return of access errors. Further unit tests will require a rewrite of unit testing infrastructure (mock URL loaders), and should probably be targeted at DocumentThreadableLoader and lower level loaders.
Comment 23 Bill Budge 2011-06-08 17:48:41 PDT
Created attachment 96517 [details]
Proposed Patch

Bad patch.
Comment 24 Adam Barth 2011-06-08 23:37:31 PDT
Comment on attachment 96517 [details]
Proposed Patch

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

Thanks for writing the test.  I appreciate it.  One nit below and this looks good to go.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:191
> +    ASSERT(timer);

Normally, we do the following:

ASSERT_UNUSED(timer == &m_errorTimer, timer);

Some configurations of WebKit generate compile errors for unused variables.  In your patch |timer| is unused in release builds.  Also, the ASSERT above is more precise because it makes sure |timer| points to the object we expect.
Comment 25 Bill Budge 2011-06-09 05:40:12 PDT
Created attachment 96574 [details]
Proposed Patch

Let's discuss bringing more of the URL loader machinery (and WebKit in general) under test at some point.
Comment 26 WebKit Review Bot 2011-06-09 05:44:33 PDT
Comment on attachment 96574 [details]
Proposed Patch

Attachment 96574 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8822317
Comment 27 Bill Budge 2011-06-09 09:54:12 PDT
Created attachment 96599 [details]
Proposed Patch

ASSERT_UNUSED didn't compile for me:

    ASSERT_UNUSED(timer == &m_errorTimer, timer);


Source/WebKit/chromium/src/AssociatedURLLoader.cpp: In member function 'void WebKit::AssociatedURLLoader::ClientAdapter::notifyError(WebCore::Timer<WebKit::AssociatedURLLoader::ClientAdapter>*)':
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:191: error: void value not ignored as it ought to be
make: *** [out/Release/obj.target/webkit/Source/WebKit/chromium/src/AssociatedURLLoader.o] Error 1

I grepped for other usages and didn't find any in Source/WebKit/chromium.
Comment 28 Darin Adler 2011-06-09 10:05:17 PDT
(In reply to comment #27)
> ASSERT_UNUSED didn't compile for me:
> 
>     ASSERT_UNUSED(timer == &m_errorTimer, timer);

That’s because Adam’s example had the arguments backwards. Instead do this:

    ASSERT_UNUSED(timer, timer == &m_errorTimer);
Comment 29 Adam Barth 2011-06-09 10:39:14 PDT
Comment on attachment 96599 [details]
Proposed Patch

What Darin said.  :)
Comment 30 Bill Budge 2011-06-09 10:47:38 PDT
Created attachment 96603 [details]
Proposed Patch

Thanks!
Comment 31 WebKit Review Bot 2011-06-09 11:22:02 PDT
Comment on attachment 96603 [details]
Proposed Patch

Clearing flags on attachment: 96603

Committed r88466: <http://trac.webkit.org/changeset/88466>
Comment 32 WebKit Review Bot 2011-06-09 11:22:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 James Kozianski 2011-06-09 23:16:42 PDT
Reverted r88466 for reason:

Broke PPAPITest.URLLoader test on the chromium bots

Committed r88523: <http://trac.webkit.org/changeset/88523>
Comment 34 Adam Barth 2011-06-16 16:13:27 PDT
Comment on attachment 96603 [details]
Proposed Patch

Lets try this again!
Comment 35 WebKit Review Bot 2011-06-16 16:53:34 PDT
Comment on attachment 96603 [details]
Proposed Patch

Clearing flags on attachment: 96603

Committed r89089: <http://trac.webkit.org/changeset/89089>
Comment 36 WebKit Review Bot 2011-06-16 16:53:41 PDT
All reviewed patches have been landed.  Closing bug.