nsIWebProgressListener never knows redirected document URL

RESOLVED FIXED in Firefox 55

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: maxim, Assigned: maxim)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Created attachment 8841645 [details] [diff] [review]
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → Document Navigation
Ever confirmed: true
Product: Firefox → Core
Version: unspecified → Trunk
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?
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]
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.
Attachment #8841645 - Flags: review?(bugs) → review+
(Assignee)

Comment 3

2 years ago
(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.
(Presumptively assigning to the fine person who attached the patch :)
Assignee: nobody → maxim
Priority: -- → P2
(Assignee)

Comment 6

2 years ago
Created attachment 8845908 [details] [diff] [review]
Updated patch

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)

Comment 9

2 years ago
Created attachment 8853405 [details] [diff] [review]
1342989.diff

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]
1342989.diff

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01cf5a8d1ad5e02244b394028f6df5927b3384c9
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.

Comment 12

2 years ago
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7d18b35f8f
Add a new flag to nsIWebProgressListener for redirects . r=smaug
https://hg.mozilla.org/mozilla-central/rev/1b7d18b35f8f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.