Issue 29350379: Issue 4386 - Update frame URL after redirection

Keyboard Shortcuts

	File
u :	up to issue
m :	publish + mail comments
M :	edit review message
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line
<Enter> :	respond to / edit current comment
d :	mark current comment as done

	Issue
u :	up to list of issues
m :	publish + mail comments
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue
# :	close issue

	Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :	save comment
<Esc> :	cancel edit

Issue 29350379: Issue 4386 - Update frame URL after redirection (Closed)

Created:
Sept. 1, 2016, 1:37 p.m. by kzar

Modified:
Sept. 13, 2016, 9:39 a.m.

Reviewers:
Wladimir Palant

CC:
Sebastian Noack

Visibility:
Public.

Description

Issue 4386 - Update frame URL after redirection

Patch Set 1 #

Total comments: 1

Created: Sept. 1, 2016, 1:37 p.m.

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+9 lines, -0 lines)			Patch
	M	chrome/ext/background.js	View		1 chunk	+9 lines, -0 lines	1 comment	Download

Messages

Total messages: 3

Expand All Messages | Collapse All Messages

kzar

https://codereview.adblockplus.org/29350379/diff/29350380/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350379/diff/29350380/chrome/ext/background.js#newcode151 chrome/ext/background.js:151: { (We could also check that details.transitionQualifiers is ["server_redirect"] ...

Sept. 1, 2016, 1:42 p.m. (2016-09-01 13:42:07 UTC) #2

Wladimir Palant

Sept. 5, 2016, 3:37 p.m. (2016-09-05 15:37:40 UTC) #3

NOT LGTM

The issue isn't limited to redirects. Just updating the URL in onCommitted
doesn't work - the approach used right now is flawed, setting URL in
onBeforeNavigate is just bad. Here is how you can see it:

1. Open JS Console on example.com.
2. Run `location.href = "http://google.com:1234/";` (server isn't listening on
this port so "loading" will spin forever).
3. Now press Esc to abort loading.
4. Run `new Image().src="http://test/";` and check "Adblock Plus" panel to see
which host this request was attributed to.

Currently Adblock Plus will set document URL to "google.com" even though that
load never happened. Consequently, any subsequent loads will be attributed to
Google and not the real site.

Expand All Messages | Collapse All Messages