1342989 - nsIWebProgressListener never knows redirected document URL

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[maxim]

Attachment: 0001-DB-699-Fixed-redirected-document-load-event-handling.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 YaBrowser/17.3.1.407 (beta) Yowser/2.5 Safari/537.36

Steps to reproduce:

When monitoring document loads, `nsIWebProgressListener` has no knowledge as to what is the new document URL after a redirect has occurred, in which case `nsDocLoader` fires notification with very generic flags `STATE_START | STATE_IS_REQUEST`.

The combination of `STATE_START | STATE_IS_DOCUMENT` flags is only sent for initial request (see `nsDocLoader::OnStartRequest`, specifically the `if (bJustStartedLoading)` branch).
And `STATE_REDIRECTING | STATE_IS_DOCUMENT` is sent with old channel (see `nsDocLoader::AsyncOnChannelRedirect`).

As a result, it is impossible to take actions based on final document URL.

Suggested patch adds another flag `STATE_IS_REDIR_DOC`, which is appended in case of a document redirect and is sent along with the new channel, providing the necessary context.

It is deliberately avoided to send technically more appropriate `STATE_REDIRECTING | STATE_IS_DOCUMENT` in order not to confuse existing browser and/or extension code, which would otherwise start receiving more of these events than they could before. This decision is subject to discussion of course.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8841645 [details] [diff] [review]
0001-DB-699-Fixed-redirected-document-load-event-handling.patch

Our partners at Cliqz use this patch in their Cliqz browser, a fork of Firefox. Does this look like something we could upstream?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8841645 [details] [diff] [review]
0001-DB-699-Fixed-redirected-document-load-event-handling.patch

Doesn't look unreasonable. Couple of nits.

>+  // This is the only way to catch document request start event after a redirect
>+  // has occured without changing inherited Firefox behaviour significantly.
>+  // Problem description:
>+  // The combination of |STATE_START + STATE_IS_DOCUMENT| is only sent for
>+  // initial request (see |doStartDocumentLoad| call above).
>+  // And |STATE_REDIRECTING + STATE_IS_DOCUMENT| is sent with old channel, which
>+  // makes it impossible to filter by destination URL (see
>+  // |AsyncOnChannelRedirect| implementation).
>+  // Fixing any of those bugs may cause unpredictable consequences in any part
>+  // of the browser, so we just add a custom flag for this exact situation.
>+  int32_t extraFlags = 0;
>+  if (mIsLoadingDocument
>+      && !bJustStartedLoading
>+      && (loadFlags & nsIChannel::LOAD_DOCUMENT_URI)
>+      && (loadFlags & nsIChannel::LOAD_REPLACE)) {
&& goes to the end of previous line, 

>+   *
>+   * STATE_IS_REDIR_DOC
>+   *  Same as STATE_IS_DOCUMENT, but sent only after a redirect has occured.
>+   *  Introduced in order not to confuse existing code with extra state change
>+   *  events. See |nsDocLoader::OnStartRequest| for more info.
The name of the new flag is a bit weird when other relevant flags use full words.
Perhaps STATE_IS_REDIRECTED_DOCUMENT

We need some tests for this.

Comment 3

[maxim]

(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 8841645 [details] [diff] [review]
> Doesn't look unreasonable. Couple of nits.

NP, I'll fix styling issues.

> We need some tests for this.

Any guides/suggestions on that? I'm only slightly familiar with Firefox tests, and never wrote on myself, so I'd be super grateful for a practical advice.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

It would probably need to be a chrome test
https://developer.mozilla.org/en/docs/Chrome_tests
and use some .sjs file to handle server side part to do redirection.

http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/browser/base/content/test/chrome/test_aboutCrashed.xul#38 might be useful-ish example, though doesn't use .sjs.
But there are test files like http://searchfox.org/mozilla-central/source/docshell/test/browser/redirect_to_example.sjs

Comment 5

[Andrew Overholt [:overholt]]

(Presumptively assigning to the fine person who attached the patch :)

Comment 6

[maxim]

Attachment: Updated patch

Hi guys.
I fixed names/formatting and also added a test, checking the expected behaviour.
Hope it helps.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8845908 [details] [diff] [review]
Updated patch

>+      dump(`onStateChange spec: ${channel.URI.spec} ` +
>+          bitFlagsToNames(flags, WEB_PROGRESS_LISTENER_FLAGS,
>+              Ci.nsIWebProgressListener) + "\n");
remove this dump()

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Hey Max, are you going to make the final change from comment 7?

Comment 9

[lucian]

Attachment: 1342989.diff

I picked up this issue - can you guys please check the new patch?

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8853405 [details] [diff] [review]
1342989.diff

Thanks Lucian, here is a try push to see if anything breaks:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01cf5a8d1ad5e02244b394028f6df5927b3384c9

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

I verified manually that the test failures are unrelated, so I will go ahead and land this.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7d18b35f8f
Add a new flag to nsIWebProgressListener for redirects . r=smaug

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1b7d18b35f8f