Intermittent issue when loading HUD tools: "cannot read property 'data' of null"

[dvas0004]

Hi All

I faced an intermittent issue when doing the following:

1. Enable HUD via icon in toolbar
2. Open configured browser (chrome in my case) by clicking on icon in toolbar

Sometimes it worked as expected and HUD is displayed, other times it would spike my CPU to 100% and chrome would freeze. Subsequent investigation showed literally thousands of errors being set in the chrome DevTools and ZAP console output, like so:

Some investigation revealed that basically the "data" of "null" came from the "loadTool" function in utils.js... the tools were not being saved in indexedDB.

Why that is turns out to be really subtle... if you check the serviceworker.js file, we used a "forEach" loop to in turn call importScripts on each default tool:

var toolScripts = [
	<<ZAP_HUD_TOOLS>>
];

// Load Tool Scripts
localforage.setItem("tools", [])
	.then(() => {
		toolScripts.forEach(script => {  // <--------- here
		 	importScripts(script); 
		});
	})

What I think the issue is, is that forEach actually calls an async function for each loop. In other words, every "importScripts" command in that forEach loop was being processed async, so the promise would return before the importScripts were all processed. This and then causes the null errors.

Turning this into plain vanilla for loop like so seems to sort out the issue on my PC, but i'll test some more and update this issue (which is serving as my braindump) to make sure it's not a red herring.

var toolScripts = [
	<<ZAP_HUD_TOOLS>>
];

localforage.setItem("tools", [])
	.then(() => {
		for (var toolScriptCounter = 0; toolScriptCounter < toolScripts.length; toolScriptCounter++){
			var script = toolScripts[toolScriptCounter];
			importScripts(script); 
		}
	})

Other solutions (if this is really the root cause) would be to use async/await in conjunction with the forEach loop

[psiinon]

Woah, good find! Will leave for @dscrobonia to confirm the issue / fix as this is def in his area ;) @dvas0004 any problems getting the HUD working other than that? Was expecting a load of questions from you before you got to this stage ;)

[dvas0004]

Not too many, but I did have a couple, so I updated the wiki (the installation section) with "Troubleshooting tips" detailing the problems I found and my solutions for them

Other than that all good so far :) :) it's well written considering all the moving parts!!!

On Thu, 4 Oct 2018, 18:23 Simon Bennetts, [email protected] wrote:

Woah, good find! Will leave for @dscrobonia https://github.com/dscrobonia to confirm the issue / fix as this is def in his area ;) @dvas0004 https://github.com/dvas0004 any problems getting the HUD working other than that? Was expecting a load of questions from you before you got to this stage ;)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/psiinon/zap-hud/issues/200#issuecomment-427060721, or mute the thread https://github.com/notifications/unsubscribe-auth/AFlDbbVwwxC-Fmt-CNWCCEycPv54b5R9ks5uhifQgaJpZM4XISau .

[dscrobonia]

this is a very interesting find! your logic seems to make sense there. curious we've never ran into this issue.

another way we can handle this is to use the Promise.all(promises) syntax where promises is an array of promises. we've done this in other places but it is a little verbose. I've recently learned that the await syntax is how all of my coworkers deal with promises and is a quite a bit cleaner. I'll see if I can put a fix in for this this weekend.

Thanks for making an issue so quickly!! : )))

[dvas0004]

To be honest it's very intermittent. Best I can tell it happens when my system is under high load. Reproducing is difficult because of it's intermittent nature - however once it happens it's easy to spot because it pegs the CPU at 100% since the browser goes into a loop trying to load tools which are "null" as shown in the above screenshots.

For completeness, the browser I'm using is launched through ZAP:

Google Chrome Version 69.0.3497.100 (Official Build) (64-bit)

[dvas0004]

This just happened again, even with my workaround. It seems like it only happens when I first start ZAP and the browser. I've change the serviceworker to cache the tools and localforage to include the tools only when the "onInstall" event fires, like so:

/* Listeners */
const onInstall = event => {
	log(LOG_INFO, 'serviceworker.install', 'Installing...');

	// Cache Files
	event.waitUntil(
		caches.open(CACHE_NAME)
			.then(cache => {
				console.log("caching urls...");
				cache.addAll(toolScripts);
				return cache.addAll(urlsToCache);
			})
			.catch(errorHandler)
	);

	// Load Tool Scripts
	localforage.setItem("tools", [])
	.then(() => {
		for (var toolScriptCounter = 0; toolScriptCounter < toolScripts.length; toolScriptCounter++){
			var script = toolScripts[toolScriptCounter];
			importScripts(script); 
		}
	})
	.then(() => {
		var ts = [];
		for (var tool in self.tools) {
			ts.push(self.tools[tool].name);
		}
		return registerTools(ts); 

	})
	.catch(errorHandler);
};

It seems better but I cannot tell for sure if this solves the problem or is just another workaround. I'll keep you updated as I get more info

Sorry for the fragmented reporting - I'm not able to consistently reproduce and everytime I update the code it seems to go away for a while... so I'm updating the issue whenever it comes

[dscrobonia]

Hey @dvas0004 just catching up on this thread. Absolutely love all your details and notes! Thank you so much for including all of that, its super helpful.

I am a little confused about what would be async in this for loop: https://github.com/psiinon/zap-hud/blob/master/src/org/zaproxy/zap/extension/hud/files/hud/serviceworker.js#L46

Isn't forEach synchronous as well as importScripts? forEach:

• https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach
• https://stackoverflow.com/questions/5050265/javascript-node-js-is-array-foreach-asynchronous

importScripts:

• https://developer.mozilla.org/en-US/docs/Web/API/WorkerGlobalScope/importScripts
• https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Using_web_workers#Importing_scripts_and_libraries

There are some major hacks/ ugly-ness in startup right now though. Trying to figure out why this bug would occur but not sure yet. I bet you're right there is some async happening somewhere though...

[dvas0004]

@dscrobonia my pleasure!

Yes you're right that both are synchronous, but regarding importScripts that only holds true if every statement and function call within the imported script is also synchronous. right? If one statement within the script is async (with no await), then it's not guaranteed to be synchronous. I haven't managed to go through every importScript to check if everything inside it synchronous, but given the nulls I've encountered, i'd assume that something inside these scripts is async and we're not awaiting the result, and so importScripts is behaving async.

Regarding forEach vs for, you got me there. I assumed that since forEach executes each loop within it's own function context, that it would be more liable to the async problem, but no - as you pointed out both methods are synchronous. I guess the giveaway should have been that the issue re-occurred even when I used a plain "for" loop - what solved it so far for me was caching and moving to "onInstall"

[dscrobonia]

ahhhh. good call on the async calls in the imported script! I didn't think of that. They all should just be a single closure followed by adding a value to an array. And within the closure it shoud just be function definitions, constants, and returning of an array. But there might be something spooky in one of those tool's files that does something async in the closure that mucks it up. hmm.....

There's something there with the fact that delaying onInstall helps but I'm not sure yet what that is. You're on to something here...

[dscrobonia]

@dvas0004 just to clarify you are running into this issue when running on a single tab right?

[dvas0004]

yes absolutely. I had literally just installed it, and it was on my first run of the HUD, no other tabs and browser was launched through ZAP, not "remotely"

On Wed, 17 Oct 2018 at 09:08, David Scrobonia [email protected] wrote:

@dvas0004 https://github.com/dvas0004 just to clarify you are running into this issue when running on a single tab right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/psiinon/zap-hud/issues/200#issuecomment-430501426, or mute the thread https://github.com/notifications/unsubscribe-auth/AFlDbWR5H855lebOhWp4aITb9kDHYQ5Qks5ulslRgaJpZM4XISau .