Fix: Avoid analyzing `/favicon.ico` multiple times #430

qzhou1607-zz · 2017-08-16T22:34:14Z

CDP already sends request to /favicon.ico automatically.
So when forcing to load favicon, we shouldn't send request
to favicon.ico again.

Fix #427

alrra · 2017-08-17T12:47:01Z

src/lib/connectors/shared/remote-debugging-connector.ts

@@ -488,7 +488,11 @@ export class Connector implements IConnector {
     * * uses `favicon.ico` and the final url after redirects.
     */
    private async getFavicon(element?: AsyncHTMLElement) {
-        const href = element ? element.getAttribute('href') : '/favicon.ico';
+        if (!element || !element.getAttribute('href')) {


There is also the case when href is empty.

Also, I don't think getFavicon should be responsible of checking these things, and instead, and at this point:

https://github.com/sonarwhal/sonar/blob/941d439affca16e3af1b0df90e739ee746df2313/src/lib/connectors/shared/remote-debugging-connector.ts#L329-L331

all the checks should be made to determine if valid favicon was specified or not.

@alrra If a page doesn't have favicon in the root directory and /favicon.ico returns 404. Do we still want to list all the warnings/errors under this url?

Do we still want to list all the warnings/errors under this url?

@qzhou1607 Sorry, I don't quite understand the question. Can you be more specific, thanks!

@alrra Like http://www.example.com/favicon.ico returns 404. In the output of running sonar against http://www.example.com, we listed:

http://www.example.com: 1 issues ... http://www.example.com/favicon.ico: 1 issues ...

But http://www.example.com/favicon.ico doesn't exist, it looks a little weird to still check its response.

But http://www.example.com/favicon.ico doesn't exist, it looks a little weird to still check its response.

Yes. However, do make sure that CDP aligns with jsdom.

qzhou1607-zz · 2017-08-17T18:37:19Z

@alrra @sarvaje I pushed up some more changes. I've talked to you both about this issue and please let me know your thought about the current solution. Here is a summary:

The cause of the duplicate messages : CDP requests favicon from the root directory automatically if no favicon is present in the html. In our previous code we didn't set the _faviconLoaded flag to true in this scenario, so getFavicon got called and fetch::end event was emitted twice.

Changes in the current solution :

If cdp has sent request to /favicon.ico already, set the _faviconLoaded flag to true as well.
getFavicon doesn't need to request /favicon.ico. This function should have never been reached in the first place if favicon is in the root directory.
getFavicon shouldn't include the code to check if the element exists. If the element doesn't exist, it shouldn't reach getFavicon.

molant

@qzhou1607 on top of the comment I left, can you fix the conflicts?

molant · 2017-08-18T13:23:10Z

src/lib/connectors/shared/remote-debugging-connector.ts

+        const hasFaviconLinkTag = hasAttributeWithValue(data.element, 'link', 'rel', 'icon');
+        // If favicon is not present in the html, CDP will send a request to get favicon from the root directory.
+        // We should set the tag to true in this case to avoid fetching favicon twice.
+        const hasFaviconInRootDir = data.request.url.endsWith('/favicon.ico');


I don't think this is right. I could very well have the following url https://sonarwhal.com/images/favicon.ico and the favicon will not be in the root dir.
We should add a test for this case too.

Then the name isn't appropriate.

I was talking with Qing, and you are right, if it isn't in the root, hasFaviconLinkTag should be true, she is going to change how to calculate hasFaviconInRootDir

@molant I think your comment is correct. There is a scenario that /images/favicon.ico is included in the html as the resource a img instead of a link tag in the head (rel set as icon). In this scenario we shouldn't set the flag to true since the favicon image isn't included in the page in an appropriate way. We should still go into getFavicon trying to manually download the favicon.

I will work on adding more tests too :)

qzhou1607-zz · 2017-08-21T17:59:08Z

@molant @alrra I added some tests to make sure that there are no duplicate requests to favicon, but some of them are failing in cdp due to a difference in the headless mode - It seems that cdp in the headless mode doesn't send requests to /favicon.ico automatically. Should I skip these tests for cdp too?

It might be relevant to this issue reported.

molant · 2017-08-21T20:52:44Z

It seems that cdp in the headless mode doesn't send requests to /favicon.ico automatically
Then I think we should download it. If by the end of the traversing nothing has happened with should manually download it.

qzhou1607-zz · 2017-08-22T23:00:26Z

I modify the code and use an approach suggested by @sarvaje before: Ignore automatic requests(and responses) from cdp to /favicon.ico to avoid emitting duplicates events. This way favicon is downloaded only once from the root and the tests work in both the headless and regular cdp. Ready for review @sarvaje @molant @alrra .

sarvaje · 2017-08-23T18:16:09Z

src/lib/connectors/jsdom/jsdom.ts

@@ -226,7 +226,7 @@ class JSDOMConnector implements IConnector {
     * * uses `favicon.ico` and the final url after redirects.
     */
    private async getFavicon(element?: HTMLElement) {
-        const href = element ? element.getAttribute('href') : '/favicon.ico';
+        const href = (element && element.getAttribute('href')) || '/favicon.ico';


Any reason to do the assignment in this way instead of the old way?

In terms of readability I prefer the old way.

@sarvaje This is due to the scenario when element exists but href is an empty string. We still want to send out a request to /favicon.ico in this case.

sarvaje · 2017-08-23T18:19:08Z

src/lib/connectors/shared/remote-debugging-connector.ts

-    private async getFavicon(element?: AsyncHTMLElement) {
-        const href = element ? element.getAttribute('href') : '/favicon.ico';
+    private async getFavicon(element: AsyncHTMLElement) {
+        const href = (element && element.getAttribute('href')) || '/favicon.ico';


same as before

sarvaje · 2017-08-23T18:28:03Z

tests/lib/connectors/collect.ts

+
+        await runTest(t, connectorBuilder, serverConfig);
+
+        t.plan(2);


t.plan is not necessary here. Take a look to this

It makes sense when you need to be sure that a block of code with t.is (or another test) is run.
For example:

t.plan(2); try { await runTest(t, connectorBuilder, serverConfig); t.is(t.context.sonar.emitAsync.withArgs('fetch::end').callCount, 1); t.is(t.context.sonar.emitAsync.withArgs('fetch::end').args[0][1].request.url, faviconInLinkElementDir); }catch(e){}

@sarvaje Done. Thanks for the information!

Fix webhintio#427

qzhou1607-zz requested a review from sarvaje August 16, 2017 22:34

qzhou1607-zz requested review from alrra, austinkelleher, molant and patrick-steele-idem as code owners August 16, 2017 22:34

alrra reviewed Aug 17, 2017

View reviewed changes

qzhou1607-zz force-pushed the fix_getfavicon_cdp branch from f46456a to 41df7f0 Compare August 17, 2017 18:25

molant requested changes Aug 18, 2017

View reviewed changes

qzhou1607-zz force-pushed the fix_getfavicon_cdp branch from 41df7f0 to fb2abc2 Compare August 19, 2017 00:58

alrra force-pushed the master branch from 55a69cb to 86c3aca Compare August 19, 2017 21:51

qzhou1607-zz force-pushed the fix_getfavicon_cdp branch from fb2abc2 to f93550f Compare August 21, 2017 17:54

qzhou1607-zz force-pushed the fix_getfavicon_cdp branch from f93550f to 70b972c Compare August 21, 2017 22:56

alrra force-pushed the master branch from f2e4883 to aa2a639 Compare August 22, 2017 13:21

qzhou1607-zz force-pushed the fix_getfavicon_cdp branch from 70b972c to 2ce9c4a Compare August 22, 2017 22:33

molant approved these changes Aug 23, 2017

View reviewed changes

sarvaje reviewed Aug 23, 2017

View reviewed changes

Fix: Avoid analyzing /favicon.ico multiple times

749e993

Fix webhintio#427

qzhou1607-zz force-pushed the fix_getfavicon_cdp branch from 2ce9c4a to 749e993 Compare August 23, 2017 19:43

sarvaje approved these changes Aug 23, 2017

View reviewed changes

molant merged commit 9eb2f98 into webhintio:master Aug 23, 2017

snyk-bot mentioned this pull request Nov 9, 2020

[Snyk] Upgrade cloudinary from 1.9.1 to 1.23.0 magnologan/sonar#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Avoid analyzing `/favicon.ico` multiple times #430

Fix: Avoid analyzing `/favicon.ico` multiple times #430

qzhou1607-zz commented Aug 16, 2017

alrra Aug 17, 2017

alrra Aug 17, 2017

qzhou1607-zz Aug 17, 2017

alrra Aug 17, 2017

qzhou1607-zz Aug 17, 2017

alrra Aug 17, 2017

qzhou1607-zz commented Aug 17, 2017

molant left a comment

molant Aug 18, 2017

molant Aug 18, 2017

sarvaje Aug 18, 2017

qzhou1607-zz Aug 18, 2017

qzhou1607-zz Aug 18, 2017

qzhou1607-zz commented Aug 21, 2017 •

edited

Loading

molant commented Aug 21, 2017

qzhou1607-zz commented Aug 22, 2017

sarvaje Aug 23, 2017 •

edited

Loading

qzhou1607-zz Aug 23, 2017 •

edited

Loading

sarvaje Aug 23, 2017

sarvaje Aug 23, 2017

sarvaje Aug 23, 2017

qzhou1607-zz Aug 23, 2017

Fix: Avoid analyzing /favicon.ico multiple times #430

Fix: Avoid analyzing /favicon.ico multiple times #430

Conversation

qzhou1607-zz commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qzhou1607-zz commented Aug 17, 2017

molant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qzhou1607-zz commented Aug 21, 2017 • edited Loading

molant commented Aug 21, 2017

qzhou1607-zz commented Aug 22, 2017

sarvaje Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

qzhou1607-zz Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix: Avoid analyzing `/favicon.ico` multiple times #430

Fix: Avoid analyzing `/favicon.ico` multiple times #430

qzhou1607-zz commented Aug 21, 2017 •

edited

Loading

sarvaje Aug 23, 2017 •

edited

Loading

qzhou1607-zz Aug 23, 2017 •

edited

Loading