Bug 60059 – AssociatedURLLoader reports some errors synchronously

• Component: Platform
• Status: RESOLVED FIXED

[Bill Budge]

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.

[Bill Budge]

Created attachment 92120 [details]
Proposed Patch

[Bill Budge]

Created attachment 92123 [details]
Proposed Patch

[Bill Budge]

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.

[Darin Fisher (:fishd, Google)]

(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 :)

[Bill Budge]

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.

[Darin Fisher (:fishd, Google)]

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

[Bill Budge]

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?

[Adam Barth]

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.

[Bill Budge]

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?

[Adam Barth]

> What do you think?

Sure.  I was just going through my inbox and took a look at the patch.

[Bill Budge]

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.

[Bill Budge]

For this to compile, LOOSE_OWN_PTR must be defined. I used adoptPtr, since the constructor for timer needs 'this'.

[Adam Barth]

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.

[Bill Budge]

Created attachment 95931 [details]
Proposed Patch

Made m_errorTimer a direct member.

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 95931 [details]
Proposed Patch

Attachment 95931 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8765128

[Adam Barth]

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

[Bill Budge]

Created attachment 95934 [details]
Proposed Patch

[Adam Barth]

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.

[Bill Budge]

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

[Adam Barth]

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.

[Bill Budge]

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.

[Bill Budge]

Created attachment 96517 [details]
Proposed Patch

Bad patch.

[Adam Barth]

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.

[Bill Budge]

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.

[WebKit Review Bot]

Comment on attachment 96574 [details]
Proposed Patch

Attachment 96574 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8822317

[Bill Budge]

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.

[Darin Adler]

(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);

[Adam Barth]

Comment on attachment 96599 [details]
Proposed Patch

What Darin said.  :)

[Bill Budge]

Created attachment 96603 [details]
Proposed Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 96603 [details]
Proposed Patch

Clearing flags on attachment: 96603

Committed r88466: <http://trac.webkit.org/changeset/88466>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Kozianski]

Reverted r88466 for reason:

Broke PPAPITest.URLLoader test on the chromium bots

Committed r88523: <http://trac.webkit.org/changeset/88523>

[Adam Barth]

Comment on attachment 96603 [details]
Proposed Patch

Lets try this again!

[WebKit Review Bot]

Comment on attachment 96603 [details]
Proposed Patch

Clearing flags on attachment: 96603

Committed r89089: <http://trac.webkit.org/changeset/89089>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.