Bug 87579 – [BlackBerry] http authentication crash the browser when user commit or cancel the http authentication dialog

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Jonathan Dong]

RIM PR: 158892

Way to reproduce:

navigate to any site which needs http authentication, input something or cancel
it directly. Then you'll see the webviews process crash.


Root cause:
We release the m_handle in the old network job too early before we start a new one with authentication information attached. when the m_handle has been passed to the new network job and the old one received a dataReceivedNotify (e.g. parsing the body data received together with 401 header), the crash happens.
So we should make sure that we cancel the current network job first, and then start the new one. by doing this we can avoid receiving a dataReceivedNotify after we start the new job.

[Jonathan Dong]

Created attachment 144203 [details]
Patch

[WebKit Review Bot]

Comment on attachment 144203 [details]
Patch

Clearing flags on attachment: 144203

Committed r118655: <http://trac.webkit.org/changeset/118655>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joe Mason]

This patch "works" in release, but causes an assertion fail in debug:

Thread 3 (CRITICAL!): ASSERTION FAILED: !findJobForHandle(guardJob)
/home/jmason/dev/webkit/Source/WebCore/platform/network/blackberry/NetworkManager.cpp(135)
: bool WebCore::NetworkManager::startJob(int, const WTF::String&,
WTF::PassRefPtr<WebCore::ResourceHandle>, const WebCore::ResourceRequest&,
BlackBerry::Platform::NetworkStreamFactory*, const WebCore::Frame&, int, int)
Thread 3 (CRITICAL!): SIGSEGV: 557486080

The cause is that in a redirect, it calls close on the original job, but then
tries to create a new job for the handle before the notifyClose is processed,
so there are briefly 2 jobs with the same handle.

[Joe Mason]

Created attachment 144842 [details]
assertion fix

2012-05-30  Joe Mason  <[e-mail]>

        [BlackBerry] Fix assertion fail on redirect due to multiple jobs per handle
        https://bugs.webkit.org/show_bug.cgi?id=87579

        Reviewed by NOBODY (OOPS!).

        RIM PR #158892:

        When we start a redirect, we now call cancelJob instead of just deleting it immediately to make sure
        that all cleanup is performed. However, we also reassign the ResourceHandle to the new job, and
        since cancelJob is asynchronous it is now assigned to two jobs simultaneously.

        Work around this by only returning handles that have not been cancelled from findJobForHandle.
        Cancelled jobs still technically exist in the jobs list, but they're invisible to callers. This is
        safe because there is literally nothing that can be done with a cancelled job - it is supposed to
        merely consume any notifications that are already in progress and then kill itself off - so no
        callers of findJobForHandle are expecting a cancelled job. (All existing callers call methods on the
        returned job which are no-ops for cancelled jobs, so there is no behaviour change.)

        No new tests because there is no behaviour change (fixes a regression).

        * platform/network/blackberry/NetworkManager.cpp:
        (WebCore::NetworkManager::findJobForHandle):

[Joe Mason]

Created attachment 144865 [details]
fix with changelogs

[George Staikos]

Comment on attachment 144865 [details]
fix with changelogs

It seems testable, but I'm not quite sure how.  There was a behavior test, hence we could have a regression test.  That said, I have no idea how to test it and the alternative is to revert the previous patch so let's go with it for now.  Suggest we put in a test if we can find one.

[Joe Mason]

(In reply to comment #7)
> (From update of attachment 144865 [details])
> It seems testable, but I'm not quite sure how.  There was a behavior test, hence we could have a regression test.  That said, I have no idea how to test it and the alternative is to revert the previous patch so let's go with it for now.  Suggest we put in a test if we can find one.

The problem was the existing tests were only run with a release build.

[WebKit Review Bot]

Comment on attachment 144865 [details]
fix with changelogs

Clearing flags on attachment: 144865

Committed r119005: <http://trac.webkit.org/changeset/119005>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.