Vis4Sense / HistoryMap

http://sensemap.io
60 stars 7 forks source link

Chrome message response (background.js and contentScripts.js) #97

Closed HumzahJavid closed 6 years ago

HumzahJavid commented 6 years ago

A chrome runtime message sent from contentScripts to background is successfully received, a response is created and sent using the callback function "sendResponse", the response is not understood by chrome.

Response generated by background.js (and sent to contentScripts)

load highlight background

Response received by contentScripts.js

load highlight contentscript

Commit which highlights this issue

JavaScript line numbering is from commit 0c6746f

kaidatavis commented 6 years ago

Hi Humzah, Can yo provide a bit more information on when this happen? Does this happen when user perform certain interaction?

Also, is the code in the repository? If so, which branch?

HumzahJavid commented 6 years ago

Hi Kai, This error appears when a url is (re)loaded while the history map is running. Upon opening a new tab, when a URL is entered and loaded, or when a tab is reloaded (with a url previously entered) the error occurs. Highlights/annotations that are present on the page do not have an effect.

The code is in the repository under branch "rebuild-mvc-content-script-model", with the latest commit titled "loadHighlights message (chrome message error)" (linked in the description above).

HumzahJavid commented 6 years ago

Hi Kai,

I was doing research into this issue, one potential suspect was from this found in the chrome documentation "This function becomes invalid when the event listener returns, unless you return true from the event listener to indicate you wish to send a response asynchronously (this will keep the message channel open to the other end until sendResponse is called)." I used this, in background,js but it didn't solve the issue, however it may cause issues further down the line

HumzahJavid commented 6 years ago

This page suggested multiple listeners may cause the issue i'm having. So whilst I was locating all chrome.runtime.onMessage listeners in order to log and analyse their requests. I identified the cause of the issue.

HumzahJavid commented 6 years ago

Cause identified

I have made a new branch "rebuild-mvc-issue97" with a commit 78df3d2 bringing attention to the file and location of the cause for issue 97. There are Two chrome runtime message listeners which run before the listeners in background.js and they send a response (the one seen in the screenshots in the opening comment) using the else. Below I have included lines 35 to 45 of login.js from the commit in the branch linked above which shows one of the listeners which can intercept and handle the "backgroundOpened" and "loadHighlights" messages

Login.js Lines 35 - 45

// handle the 'login' button on historyMap
chrome.runtime.onMessage.addListener(function (request, sender, sendResponse) {
    //issue #97 cause identified here 
    console.log("login got a message " + JSON.stringify(request)); 
    if (request.text === "login") {
        googleLogin();
        sendResponse({text:'using google+ login'})
    } else {
        sendResponse({text:'do not understand the message' + request.text});
    }
});

I will attempt to resolve the issue in this branch, but I would need someone from the backend to test the code, to make sure my solution doesn't break the login code.

kaidatavis commented 6 years ago

Humzah, thanks for the update. It is good to know that you solved the problem, thought I didn't quite follow what the cause and solution are, something we can discuss this afternoon. Also, Reday will be there as well so we can test if it affects login.

HumzahJavid commented 6 years ago

This commit fixes the issue d262653 by removing an else statement if chrome.runtime message listeners

RedayY commented 6 years ago

Hello Humzah,

I just went through all of the E-Mails,

Let’s talk about this tomorrow if you like, so I can get a clear understanding on what needs to be done ?

-Reday

Am 03.02.2018 um 12:25 schrieb HumzahJavid notifications@github.com<mailto:notifications@github.com>:

Cause identified

I have made a new branch "rebuild-mvc-issue97" with a commit 78df3d2https://github.com/Vis4Sense/SenseMapExtension/commit/78df3d2680ff7ddedda4afcbee34878f208835bb bringing attention the file and location of the cause. There are Two chrome runtime message listeners which run before the listeners in background.js and they send a response (the one seen in the screenshots in the opening comment) using the else. Below I have included lines 35 to 45 of login.js from the commit in the branch linked above which shows one of the listeners which can intercept and handle the "backgroundOpened" and "loadHighlights" messages

// handle the 'login' button on historyMap chrome.runtime.onMessage.addListener(function (request, sender, sendResponse) { //issue #97 cause identified here console.log("login got a message " + JSON.stringify(request)); if (request.text === "login") { googleLogin(); sendResponse({text:'using google+ login'}) } else { sendResponse({text:'do not understand the message' + request.text}); } });

I will attempt to resolve the issue in this branch, but I would need someone from the backend to test the code, to make sure my solution doesn't break the login code.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/Vis4Sense/SenseMapExtension/issues/97#issuecomment-362802452, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AQXZKgeBTHFq28EaE6Tpi1AlTOqfdbzrks5tRFAmgaJpZM4RyQjw.