PDF Viewer generates 'pdf.js' DNS query requests

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: yury, Assigned: jll544, Mentored)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 4 obsolete attachments)

Reporter

Description

5 years ago
STR (on Windows):
1. Open Microsoft Network Monitor
2. Start "New Capture", enter `Conversation.DnsName == "pdf.js"` in the Display Filter, "Apply" it, and click "Start" button
3. Open Firefox
4. Navigate to a PDF e.g. http://www.education.gov.yk.ca/pdf/pdf-test.pdf

Actual result:
Notice couple of frames entries at "Frame Summary" window

Expected result:
No DNS traffic with pdf.js host


See also:
  https://groups.google.com/forum/#!topic/mozilla.dev.pdf-js/dTJBAPMixtA
  https://groups.google.com/forum/#!topic/mozilla.dev.pdf-js/-g1dSeT5iZE
  http://logs.glob.uno/?c=mozilla%23pdfjs#c27428
Reporter

Comment 1

5 years ago
Visible also with
 NSPR_LOG_MODULES=nsHostResolver:5 ./mach run
Reporter

Comment 2

5 years ago
(lldb) p host
(const char *) $2 = 0x00000001193bebb8 "pdf.js"
(lldb) bt
* thread #1: tid = 0x33919c, 0x00000001018ebf24 XUL`nsHostResolver::ResolveHost(this=0x000000010031c000, host=0x00000001193bebb8, flags=24, af=0, callback=0x0000000119512640) + 84 at nsHostResolver.cpp:727, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x00000001018ebf24 XUL`nsHostResolver::ResolveHost(this=0x000000010031c000, host=0x00000001193bebb8, flags=24, af=0, callback=0x0000000119512640) + 84 at nsHostResolver.cpp:727
    frame #1: 0x00000001018f86b4 XUL`nsDNSService::AsyncResolve(this=0x000000010033ba00, aHostname=0x00007fff5fbfbfa8, flags=24, listener=0x000000011435bca0, target_=0x0000000000000000, result=0x00007fff5fbfbec0) + 1380 at nsDNSService2.cpp:730
    frame #2: 0x00000001040c66c0 XUL`nsHTMLDNSPrefetch::nsDeferrals::SubmitQueue(this=0x0000000116d8a000) + 864 at nsHTMLDNSPrefetch.cpp:324
    frame #3: 0x00000001040c68fb XUL`nsHTMLDNSPrefetch::nsDeferrals::OnStateChange(this=0x0000000116d8a000, aWebProgress=0x0000000124936828, aRequest=0x000000011c349058, progressStateFlags=131088, aStatus=NS_OK) + 187 at nsHTMLDNSPrefetch.cpp:402
    frame #4: 0x000000010281b47d XUL`nsDocLoader::DoFireOnStateChange(this=0x00000001003c0ba0, aProgress=0x0000000124936828, aRequest=0x000000011c349058, aStateFlags=0x00007fff5fbfc140, aStatus=NS_OK) + 669 at nsDocLoader.cpp:1269
    frame #5: 0x000000010281af1d XUL`nsDocLoader::doStopDocumentLoad(this=0x0000000124936800, request=0x000000011c349058, aStatus=NS_OK) + 333 at nsDocLoader.cpp:850
    frame #6: 0x0000000102819abb XUL`nsDocLoader::DocLoaderIsEmpty(this=0x0000000124936800, aFlushLayout=true) + 987 at nsDocLoader.cpp:740
    frame #7: 0x000000010281a967 XUL`nsDocLoader::OnStopRequest(this=0x0000000124936800, aRequest=0x00000001286e6660, aCtxt=0x0000000000000000, aStatus=NS_OK) + 1303 at nsDocLoader.cpp:624
    frame #8: 0x000000010281ad4d XUL`non-virtual thunk to nsDocLoader::OnStopRequest(this=0x0000000124936808, aRequest=0x00000001286e6660, aCtxt=0x0000000000000000, aStatus=NS_OK) + 61 at nsDocLoader.cpp:628
    frame #9: 0x000000010183e0a6 XUL`nsLoadGroup::RemoveRequest(this=0x00000001245e4010, request=0x00000001286e6660, ctxt=0x0000000000000000, a

I think it's connected with the HTTP DNS prefetch functionality, which does not really ignore URIs that are not valid web resources: http://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDNSPrefetch.cpp#319
Reporter

Comment 3

5 years ago
Setting 'network.dns.disablePrefetch' fixes the issue.
Component: PDF Viewer → Networking
Product: Firefox → Core
(In reply to Yury Delendik (:yury) from comment #2)
> (lldb) p host
> (const char *) $2 = 0x00000001193bebb8 "pdf.js"
> (lldb) bt
> * thread #1: tid = 0x33919c, 0x00000001018ebf24
> XUL`nsHostResolver::ResolveHost(this=0x000000010031c000,
> host=0x00000001193bebb8, flags=24, af=0, callback=0x0000000119512640) + 84
> at nsHostResolver.cpp:727, queue = 'com.apple.main-thread', stop reason =
> breakpoint 1.1
>   * frame #0: 0x00000001018ebf24
> XUL`nsHostResolver::ResolveHost(this=0x000000010031c000,
> host=0x00000001193bebb8, flags=24, af=0, callback=0x0000000119512640) + 84
> at nsHostResolver.cpp:727
>     frame #1: 0x00000001018f86b4
> XUL`nsDNSService::AsyncResolve(this=0x000000010033ba00,
> aHostname=0x00007fff5fbfbfa8, flags=24, listener=0x000000011435bca0,
> target_=0x0000000000000000, result=0x00007fff5fbfbec0) + 1380 at
> nsDNSService2.cpp:730
>     frame #2: 0x00000001040c66c0
> XUL`nsHTMLDNSPrefetch::nsDeferrals::SubmitQueue(this=0x0000000116d8a000) +
> 864 at nsHTMLDNSPrefetch.cpp:324
>     frame #3: 0x00000001040c68fb
> XUL`nsHTMLDNSPrefetch::nsDeferrals::OnStateChange(this=0x0000000116d8a000,
> aWebProgress=0x0000000124936828, aRequest=0x000000011c349058,
> progressStateFlags=131088, aStatus=NS_OK) + 187 at nsHTMLDNSPrefetch.cpp:402
>     frame #4: 0x000000010281b47d
> XUL`nsDocLoader::DoFireOnStateChange(this=0x00000001003c0ba0,
> aProgress=0x0000000124936828, aRequest=0x000000011c349058,
> aStateFlags=0x00007fff5fbfc140, aStatus=NS_OK) + 669 at nsDocLoader.cpp:1269
>     frame #5: 0x000000010281af1d
> XUL`nsDocLoader::doStopDocumentLoad(this=0x0000000124936800,
> request=0x000000011c349058, aStatus=NS_OK) + 333 at nsDocLoader.cpp:850
>     frame #6: 0x0000000102819abb
> XUL`nsDocLoader::DocLoaderIsEmpty(this=0x0000000124936800,
> aFlushLayout=true) + 987 at nsDocLoader.cpp:740
>     frame #7: 0x000000010281a967
> XUL`nsDocLoader::OnStopRequest(this=0x0000000124936800,
> aRequest=0x00000001286e6660, aCtxt=0x0000000000000000, aStatus=NS_OK) + 1303
> at nsDocLoader.cpp:624
>     frame #8: 0x000000010281ad4d XUL`non-virtual thunk to
> nsDocLoader::OnStopRequest(this=0x0000000124936808,
> aRequest=0x00000001286e6660, aCtxt=0x0000000000000000, aStatus=NS_OK) + 61
> at nsDocLoader.cpp:628
>     frame #9: 0x000000010183e0a6
> XUL`nsLoadGroup::RemoveRequest(this=0x00000001245e4010,
> request=0x00000001286e6660, ctxt=0x0000000000000000, a
> 
> I think it's connected with the HTTP DNS prefetch functionality, which does
> not really ignore URIs that are not valid web resources:
> http://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDNSPrefetch.
> cpp#319

sworkman, can you comment on Yury's analysis above? I think it's your name on the checkin for those lines?
Flags: needinfo?(sworkman)
Reporter

Comment 5

5 years ago
Sorry, forgot to mention that URL, it is trying to use in the DNS prefetch, starts with "resource://pdf.js/".
Reporter

Comment 6

5 years ago
Posted file Minimal test case (obsolete) —
Reporter

Comment 7

5 years ago
Comment on attachment 8523060 [details]
Minimal test case

http://people.mozilla.org/~ydelendik/tmp/bug1098414_test.html
Attachment #8523060 - Attachment description: t.html → Minimal test case
Attachment #8523060 - Attachment is obsolete: true
DNS prefetches for links like this don't take the scheme into consideration. In fact, they discount it altogether and just focus on the hostname:
http://dxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDNSPrefetch.cpp#301

So, if the URI is resource://pdf.js/ then yes, I would expect a lookup request for pdf.js. Note that the minimal test case in comment #7 has a different resource: host name - do you still see a pdf.js lookup for that one too?

Now, in my opinion this seems harmless, but I could be wrong. Is there some bad effect that I'm not thinking about?

As far as potential solutions, URI's starting with resource: could be filtered out. If you want to supply a patch I can review it :)
Flags: needinfo?(sworkman)
Probably testing nsIProtocolHandler::URI_IS_LOCAL_RESOURCE would be better than hard-coded scheme names.
Ah, yes, better indeed :)
Reporter

Comment 11

5 years ago
(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #8)
> Note that the minimal test case in comment #7 has a
> different resource: host name - do you still see a pdf.js lookup for that
> one too?

No, but I see lookup for `fakehost.example.org`. (The test just demonstrates that the issue is in the Gecko core and not with pdf.js)

> Now, in my opinion this seems harmless, but I could be wrong. Is there some
> bad effect that I'm not thinking about?

Only unneeded DNS queries.

> 
> As far as potential solutions, URI's starting with resource: could be
> filtered out. If you want to supply a patch I can review it :)

Is it easy patch? If it is, please set "good first bug" in the whiteboard field.

Personally I don't fully get the proposed solution. What about "chrome://"? Why not whitelist only "http" and "https" (maybe "file" and "ftp")?
Flags: needinfo?(sworkman)
(In reply to Yury Delendik (:yury) from comment #11)
> > As far as potential solutions, URI's starting with resource: could be
> > filtered out. If you want to supply a patch I can review it :)
> 
> Is it easy patch? If it is, please set "good first bug" in the whiteboard
> field.

Good idea. Done.

> Personally I don't fully get the proposed solution. What about "chrome://"?
> Why not whitelist only "http" and "https" (maybe "file" and "ftp")?

nsIProtocolHandler::URI_IS_LOCAL_RESOURCE (see :emk's comment #9) is better here and should effectively do what you're suggesting.
http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIProtocolHandler.idl#221
Flags: needinfo?(sworkman)
Whiteboard: good first bug
OK, if this is a good first bug I'm going to farm it out to the community. Steve, do you feel like mentoring a contributor through this?
Flags: needinfo?(sworkman)
Whiteboard: good first bug → [good first bug]
I'm going to ask Dragana if she'd like to do it first.

Dragana, if you want to pass on this, I'll take it.
Flags: needinfo?(sworkman) → needinfo?(dd.mozilla)
I will take it.
Flags: needinfo?(dd.mozilla)
Thanks Dragana! I've added you to the mentors list for the bug. (Mike, you can fix that if I've done it wrong!)
Mentor: dd.mozilla
Looks good, let's run with it.
(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #12)
> http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/
> nsIProtocolHandler.idl#221

researching this bug out of curiosity and the above url is 404 ?
the public level has been flattened.. its now netwerk/base/nsI..
Curtis, do you want this one?
(In reply to Mike Hoye [:mhoye] from comment #20)
> Curtis, do you want this one?

I wanted to take a look at the code first to see if I understand what needs to be changed before I committed to taking it. Yes, my goal was to to take it.
Assignee

Comment 22

4 years ago
Posted patch bug-1098415.patch (obsolete) — Splinter Review
Skip DNS prefetch for links with nsIProtocolHandler::URI_IS_LOCAL_RESOURCE flag.
Attachment #8561547 - Flags: review?(dd.mozilla)
Comment on attachment 8561547 [details] [diff] [review]
bug-1098415.patch

Review of attachment 8561547 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good just a minor thing.

::: dom/html/nsHTMLDNSPrefetch.cpp
@@ +316,4 @@
>            hrefURI->GetAsciiHost(hostName);
> +          NS_URIChainHasFlags(hrefURI,
> +                              nsIProtocolHandler::URI_IS_LOCAL_RESOURCE,
> +                              &isLocalResource);

Check return value from NS_URIChainHasFlags.
Since this is just prefetch, I would not fetch dns in the case of an error.
Attachment #8561547 - Flags: review?(dd.mozilla) → feedback+
Assignee

Comment 24

4 years ago
Posted patch bug-1098415-v2.patch (obsolete) — Splinter Review
Added retval check for NS_URIChainHasFlags. Also, truncate hostName for each pass through the loop (otherwise hostName.isEmpty might be checking the result of a previous iteration).
Attachment #8561547 - Attachment is obsolete: true
Attachment #8562958 - Flags: review?(dd.mozilla)
Comment on attachment 8562958 [details] [diff] [review]
bug-1098415-v2.patch

Review of attachment 8562958 [details] [diff] [review]:
-----------------------------------------------------------------

Loos gode just nits

::: dom/html/nsHTMLDNSPrefetch.cpp
@@ +328,5 @@
>                                             mEntries[mTail].mFlags);
>            } else {
>              nsCOMPtr<nsICancelable> tmpOutstanding;
>  
> +            rv = sDNSService->AsyncResolve(hostName, 

nit: black at the end of the line.

@@ +333,4 @@
>                                      mEntries[mTail].mFlags
>                                      | nsIDNSService::RESOLVE_SPECULATE,
>                                      sDNSListener, nullptr,
>                                      getter_AddRefs(tmpOutstanding));

nit: When you already changing this can you align this lines with "hostName"
Attachment #8562958 - Flags: review?(dd.mozilla) → review+
Assignee: nobody → jll544
Status: NEW → ASSIGNED
Assignee

Comment 26

4 years ago
Whitespace fixed per review.

I'm new to the process here... do I need to flag for review again, or anything else?  Thanks.
Attachment #8562958 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Usually you would just mark it as review +, because my last comments are really just not important stuff.

But since I am not a peer for necko and for all (or the most of) patches you need a peer to review it, I will ask Steve to review it :)
Flags: needinfo?(dd.mozilla)
Comment on attachment 8563708 [details] [diff] [review]
bug-1098415-v3.patch

Steve can you take a look at this. It is really short.
The sheriff thought that it needs a peer review.
Attachment #8563708 - Flags: review?(sworkman)
Comment on attachment 8563708 [details] [diff] [review]
bug-1098415-v3.patch

Review of attachment 8563708 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me except for one thing. There is another call to AsyncResolve that doesn't go through the deferrals queue. It's in PrefetchLow(string) and is called from nsContentSink. It would require the string being passed in to be converted to a URI first (NS_NewURI) and then checking the flags. I think that should work. I suggest doing that in a second patch. This one is good and is the most important one, so we should get it landed now.
Attachment #8563708 - Flags: review?(sworkman) → review+
Do you want to do the second patch, described in comment #29?
If you want do it as a separate patch.
Flags: needinfo?(jll544)
Jeff, 
I made a try run (I assumed you cannot submit to the try server).
So you can now ask for checkin (put the keyword checkin-needed) and also put keyword leave-open so that they do not close this bug and we can fix the issue from comment #29.
Assignee

Comment 33

4 years ago
Dragana,
Thanks for the try run... it's true I can't do it myself. :)
I'll start looking at creating a second patch for PrefetchLow().
Flags: needinfo?(jll544)
(In reply to Jeff Lu from comment #33)
> Dragana,
> Thanks for the try run... it's true I can't do it myself. :)

get yourself l1 hg access.. I'm happy to vouch for anyone who written a sensible patch!

https://www.mozilla.org/en-US/about/governance/policies/commit/

trust me, its a huge productivity boon.
Assignee

Comment 37

4 years ago
I need some guidance to know if I'm on the right track for comment #29...

For nsHTMLDNSPrefetch::PrefetchLow(string), the string is just a hostname, so it seems there would not be enough information to convert to a URI, right? (scheme is unknown in this scope)
http://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDNSPrefetch.cpp#156

So, the place to check URI flags would be in the caller, nsContentSink::PrefetchDNS.  Checking there for URI_IS_LOCAL_RESOURCE would be easy, because the function already converts to URI to obtain the hostname:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentSink.cpp#846

Would that be the right approach, or am I missing something?
Also, is there an easy way to reach this code (nsContentSink::PrefetchDNS) to reproduce the unwanted behavior?  I tried setting a breakpoint there, but I don't seem to be hitting it by loading the URLs in comment #7 and comment #0 (or any other URL, for that matter).
Flags: needinfo?(dd.mozilla)
(In reply to Jeff Lu from comment #37)
> I need some guidance to know if I'm on the right track for comment #29...
> 
> For nsHTMLDNSPrefetch::PrefetchLow(string), the string is just a hostname,
> so it seems there would not be enough information to convert to a URI,
> right? (scheme is unknown in this scope)
> http://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDNSPrefetch.
> cpp#156
> 
> So, the place to check URI flags would be in the caller,
> nsContentSink::PrefetchDNS.  Checking there for URI_IS_LOCAL_RESOURCE would
> be easy, because the function already converts to URI to obtain the hostname:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentSink.cpp#846
> 
> Would that be the right approach, or am I missing something?

It can be done like that and it is correct, but maybe move the check into nsHTMLDNSPrefetch and change the parameter name to aUriStr.

Actually probably it needs a lot of changes for that because of all these functions PrefetchLow, PrefetchMedium...
so you can do the check in nsContentSink. And please check if there are other calls of PrefetchLow, PrefetchMedium, PrefetchHigh and Prefetch (taking a short look there is none, except one other in NeckoParent.cpp which does not need a check).  

> Also, is there an easy way to reach this code (nsContentSink::PrefetchDNS)
> to reproduce the unwanted behavior?  I tried setting a breakpoint there, but
> I don't seem to be hitting it by loading the URLs in comment #7 and comment
> #0 (or any other URL, for that matter).

Comments #7 and #0 hit the part you have fixed in last patch.
You can try making a test similar to #7 and look which parsers use nsContentSink::PrefetchDNS(const nsAString &aHref).
Flags: needinfo?(dd.mozilla)
Assignee

Comment 39

4 years ago
Posted patch bug-1098415-part2.patch (obsolete) — Splinter Review
Attached patch implements the check for URI_IS_LOCAL_RESOURCE inside nsContentSink. I wasn't able to find any other calls to PrefetchLow/Medium/High(string).

Simple test case is to insert into a web page:
<link rel="dns-prefetch" href="resource://fakehost.example.org">
Attachment #8575790 - Flags: review?(dd.mozilla)
Comment on attachment 8575790 [details] [diff] [review]
bug-1098415-part2.patch

Review of attachment 8575790 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.
You can flag this for Steve to review it too.
In the commit message you can put r=name of review that is going to review the patch (e.g. r=sworkman, r=dragana)
Attachment #8575790 - Flags: review?(dd.mozilla) → review+
Assignee

Comment 41

4 years ago
Updated the commit message with reviewers.
Steve, could you take a look?
Attachment #8575790 - Attachment is obsolete: true
Attachment #8576712 - Flags: review?(sworkman)
Comment on attachment 8576712 [details] [diff] [review]
bug-1098415-part2.patch

Review of attachment 8576712 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! Thanks Jeff and Dragana.
Attachment #8576712 - Flags: review?(sworkman) → review+
https://hg.mozilla.org/mozilla-central/rev/10182fa9ce5a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.