Bug 63591 - [Chromium] isRedirect is incorrectly determined for decidePolicyForNavigation
Summary: [Chromium] isRedirect is incorrectly determined for decidePolicyForNavigation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-28 18:15 PDT by Ben Smith
Modified: 2011-09-06 20:00 PDT (History)
2 users (show)

See Also:


Attachments
Patch to fix this bug. (1.30 KB, patch)
2011-06-28 18:15 PDT, Ben Smith
no flags Details | Formatted Diff | Diff
Patch (4.57 KB, patch)
2011-08-30 15:40 PDT, Ben Smith
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Smith 2011-06-28 18:15:33 PDT
Created attachment 99017 [details]
Patch to fix this bug.

In FrameLoaderClientImpl::dispatchDecidePolicyForNavigationAction()

isRedirect is currently determined by hasRedirectChain(), which checks for an non-empty redirect chain. Instead, it should check for a redirect chain with more than one element. All valid redirects will have at least the source URL and the destination URL in the redirect chain.
Comment 1 Ben Smith 2011-06-28 18:17:30 PDT
I don't believe a LayoutTest is necessary for this patch, because isRedirect is only used in chromium (for determining whether to fork a process), and has no user-visible effect in the browser.
Comment 2 Adam Barth 2011-06-29 11:48:05 PDT
Comment on attachment 99017 [details]
Patch to fix this bug.

All patch require a ChangeLog:

http://www.webkit.org/coding/contributing.html
Comment 3 Adam Barth 2011-06-29 11:51:07 PDT
Comment on attachment 99017 [details]
Patch to fix this bug.

Is this testable with a Chromium WebKit API test?  Generally, these sorts of subtle changes are easy to regress if we don't have any test coverage.  If this is used for forking processes in the embedder, perhaps we can write a test at that layer?

(Marking r- because of the lack of changelog.)
Comment 4 Ben Smith 2011-06-29 12:10:28 PDT
Yes, I believe it would be possible to test at that level.

Is it WebKit/Source/WebKit/chromium/tests/*?
Comment 5 Ben Smith 2011-08-30 15:40:22 PDT
Created attachment 105710 [details]
Patch
Comment 6 Ben Smith 2011-08-31 14:36:29 PDT
It's been a while since I've had time to work on this one... :)

Adam, can you review my new patch? I've included a test in WebFrameTest that exhibits the bug (when the fix in FrameLoaderClientImpl.cpp is removed).
Comment 7 Adam Barth 2011-08-31 14:44:06 PDT
Comment on attachment 105710 [details]
Patch

Looks great.  Thanks for adding the ChangeLog and test.
Comment 8 Ben Smith 2011-09-06 18:47:10 PDT
Adam, is there something else I need to do to commit this patch? It seems that it is in an infinite commit-queue. :)
Comment 9 Adam Barth 2011-09-06 19:33:53 PDT
Comment on attachment 105710 [details]
Patch

We just need to approve it for the queue!
Comment 10 WebKit Review Bot 2011-09-06 20:00:01 PDT
Comment on attachment 105710 [details]
Patch

Clearing flags on attachment: 105710

Committed r94630: <http://trac.webkit.org/changeset/94630>
Comment 11 WebKit Review Bot 2011-09-06 20:00:06 PDT
All reviewed patches have been landed.  Closing bug.