Closed
Bug 1342989
Opened 7 years ago
Closed 7 years ago
nsIWebProgressListener never knows redirected document URL
Categories
(Core :: DOM: Navigation, defect, P2)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: maxim, Assigned: maxim)
Details
Attachments
(2 files, 1 obsolete file)
10.33 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
10.15 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•7 years ago
|
Status: UNCONFIRMED → NEW
Component: Untriaged → Document Navigation
Ever confirmed: true
Product: Firefox → Core
Version: unspecified → Trunk
Comment 1•7 years ago
|
||
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 2•7 years ago
|
||
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+
(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•7 years ago
|
||
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•7 years ago
|
||
(Presumptively assigning to the fine person who attached the patch :)
Assignee: nobody → maxim
Priority: -- → P2
Hi guys. I fixed names/formatting and also added a test, checking the expected behaviour. Hope it helps.
Attachment #8841645 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8845908 -
Flags: review?(bugs)
Comment 7•7 years ago
|
||
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+
Comment 8•7 years ago
|
||
Hey Max, are you going to make the final change from comment 7?
Flags: needinfo?(maxim)
I picked up this issue - can you guys please check the new patch?
Attachment #8853405 -
Flags: review?(past)
Comment 10•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
I verified manually that the test failures are unrelated, so I will go ahead and land this.
Comment 12•7 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
![]() |
||
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1b7d18b35f8f
Status: NEW → RESOLVED
Closed: 7 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.
Description
•