Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(49)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 2 months ago by kzar
Modified:
3 years, 2 months ago
Reviewers:
Wladimir Palant
CC:
Sebastian Noack
Visibility:
Public.

Description

Issue 4386 - Update frame URL after redirection

Patch Set 1 #

Total comments: 1
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
kzar
Patch Set 1
3 years, 2 months ago (2016-09-01 13:38:16 UTC) #1
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"] ...
3 years, 2 months ago (2016-09-01 13:42:07 UTC) #2
Wladimir Palant
3 years, 2 months ago (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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5