Closed Bug 1342989 Opened 7 years ago Closed 7 years ago

nsIWebProgressListener never knows redirected document URL


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




Tracking Status
firefox55 --- fixed


(Reporter: maxim, Assigned: maxim)



(2 files, 1 obsolete file)

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/ (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.
Component: Untriaged → Document Navigation
Ever confirmed: true
Product: Firefox → Core
Version: unspecified → Trunk
Comment on attachment 8841645 [details] [diff] [review]

Our partners at Cliqz use this patch in their Cliqz browser, a fork of Firefox. Does this look like something we could upstream?
Attachment #8841645 - Attachment is patch: true
Attachment #8841645 - Attachment mime type: text/x-patch → text/plain
Attachment #8841645 - Flags: review?(bugs)
Comment on attachment 8841645 [details] [diff] [review]

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, 

>+   *
>+   *  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.

We need some tests for this.
Attachment #8841645 - Flags: review?(bugs) → review+
(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.
It would probably need to be a chrome test
and use some .sjs file to handle server side part to do redirection. might be useful-ish example, though doesn't use .sjs.
But there are test files like
(Presumptively assigning to the fine person who attached the patch :)
Assignee: nobody → maxim
Priority: -- → P2
Attached patch Updated patchSplinter Review
Hi guys.
I fixed names/formatting and also added a test, checking the expected behaviour.
Hope it helps.
Attachment #8841645 - Attachment is obsolete: true
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()
Attachment #8845908 - Flags: review?(bugs) → review+
Hey Max, are you going to make the final change from comment 7?
Flags: needinfo?(maxim)
Attached patch 1342989.diffSplinter Review
I picked up this issue - can you guys please check the new patch?
Attachment #8853405 - Flags: review?(past)
Comment on attachment 8853405 [details] [diff] [review]

Thanks Lucian, here is a try push to see if anything breaks:
Flags: needinfo?(maxim)
Attachment #8853405 - Flags: review?(past)
I verified manually that the test failures are unrelated, so I will go ahead and land this.
Pushed by
Add a new flag to nsIWebProgressListener for redirects . r=smaug
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.