Margaryta Chepiga
Mar 26, 2018 · 9 min read

Type of the Release

This release is regarding the bug fix. The issue page for the bug is here.

How it was Chosen

As I mentioned in my previous release, this issue was suggested by David Humphrey.

What is the Expected Result

Before:

The problem was that when a user tries to download something from the website, the icon in URL bar disappears. The expected result is for the icon not to disappear.

Process & Solution

It took me more than three weeks to come to the fix. Moreover, it took me more than two weeks to get to the meaningful progress. I have to admit, this was not an easy bug for me. I struggled a lot. I would struggle for a much longer time if I was left by myself out there.

Teamwork is your friend.

After introducing the bug to my peers and my professor David Humphrey, they all helped me to debug it and look for possible ways to solve it. Then one of my classmates Marco offered me help. That’s not where it started, but that’s where the real progress happened.

Credo of the current release: “Teamwork makes the fix work”

By Marco Beltempo

I don’t think I ever was as happy when I fixed a bug as I was when we fixed this issue.

Proof:

The second it got fixed, I was crying and laughing at the same time. That is the reaction you get for something that you spend not just hours, but days working on non-stop. When it wasn’t working ( or wasn’t fixed properly ) for so long after you debugged that much lines of code. Attempt after attempt, I was questioning myself if I would ever fix it. So many times when from time to time I almost gave up. All that was totally worth the moment. Hard work pays off.

What is more important in this release though is the reason why this was even fixed. David Humphrey always encouraged us (students) to work together and help each other out. The more I follow his suggestions, the more I can see that all of them work, and I can notice the result of following them. I think that the experience of working together, discussing our ideas, sharing our thought process was even more important experience than the fix itself (no matter how good it feels to fix something). We spend the entire weekend with Marco working on the bug. It was a truly amazing experience! We were working together and separate at the same time. Isn’t it magnificent how two people can make more progress than one? How talking and discussing the problem and possible solutions can bring you to the answer? I wouldn’t come to the solution as fast as I came up with Marco to one. Different perspective, different think process, and good communication, isn’t it a solution? It was for us.

It started with Marco.

He made a console.log in this function:

get iconCssClasses () {if (this.props.isPotentialPhishingUrl) {return ['fa-exclamation-triangle', 'insecure-color']} else if (this.isSearch) {return ['fa-search']} else if (this.props.isAboutPage && !this.props.titleMode) {return ['fa-list']} else if (this.props.isHTTPPage && !this.props.active) {console.log("isSecure: ", this.props.isSecure)if (this.props.isSecure === true) {return ['fa-lock']} else if (this.props.isSecure === false || this.props.isSecure === 2) {return ['fa-unlock', 'insecure-color']} else if (this.props.isSecure === 1) {return ['fa-unlock']}}return []}

Which in developers tools showed the following (to open dev tools in Brave press shift+F8) :

As you can see, in the function we are checking if isSecure is equals to true, false, 2 and 1. But when we refresh the page or click on download the value of isSecure is null. But there is no case for null. Therefore, Marco added the check for null:

get iconCssClasses () {if (this.props.isPotentialPhishingUrl) {return ['fa-exclamation-triangle', 'insecure-color']} else if (this.isSearch) {return ['fa-search']} else if (this.props.isAboutPage && !this.props.titleMode) {return ['fa-list']} else if (this.props.isHTTPPage && !this.props.active) {console.log("isSecure: ", this.props.isSecure)if (this.props.isSecure === true || this.props.isSecure === null) {return ['fa-lock']} else if (this.props.isSecure === false || this.props.isSecure === 2 || this.props.isSecure === null) {return ['fa-unlock', 'insecure-color']} else if (this.props.isSecure === 1 || this.props.isSecure === null) {return ['fa-unlock']}}return []}

If we run the changes in the browser we will see the following:

Since I have experience with edge cases, that was the first thing that I did when I saw Marco’s solution. It wasn’t covering all the cases. As you can see from the GIF above, this works as expected for insecure websites and secure websites without the certificate. However, for the secure connection with the certificate when we try to download the certificate disappears. Even though it wasn’t the final solution, it was such an amazing progress! Cause this exact console.log and the null check made us going so much further. Having this information and this progress, gave a big push for the solution. He did a perfect console log which showed that isSecure property for the URL bar icon was returning null. This was a big hit. This moment encouraged me to move forward.

Next step was to search where isSecure is being set to null.

After more debugging, we found out that in frame.js on line 691 (as you can see on the above ) if we will change the value of isSecure, the result won’t change. Since we knew that the isSecure becomes null somewhere. We needed to find where exactly. First, we looked for every isSecure = null, after that we run the console log to see the if it is called there, then we changed the values of isSecure to see if that affects anything. It wasn’t. But, we found in the same file, a little below (on line 721), we are calling a function to change the state:

windowActions.setSecurityState(this.props.tabId, {secure: runInsecureContent ? false : isSecure,runInsecureContent,evCert})

Therefore, next step was to look for setSecurityState. However, that didn’t give us much. After hours of debugging though, we found windowStore.js.

In windowStore.js, there is a function which resets the state:

// Reset security statewindowState =windowState.deleteIn(statePath('frames').concat(['security', 'blockedRunInsecureContent']))windowState = windowState.mergeIn(statePath('frames').concat(['security']), {isSecure: null,evCert: undefined,runInsecureContent: false})

When we changed the value of isSecure from null to true. Guess what happened. Now, isSecure was never null.

So okay, this is closer to what we want. But what are we actually suppose to do? If I change this value to be secure, then it will always be secure and vise versa. This wasn’t really a solution.

Then we tried something interesting, we tried to comment out that code at all. The result was totally surprised at least for me.

// Reset security statewindowState =windowState.deleteIn(statePath('frames').concat(['security', 'blockedRunInsecureContent']))windowState = windowState.mergeIn(statePath('frames').concat(['security']), {/* isSecure: null,evCert: undefined,runInsecureContent: false*/})

It actually worked!

However, the comment for the function kinda was telling us that this function has a purpose and it is supposed to be there. It said that it is for resetting the state, but we needed to know more. Therefore, we went to GitHub to that function and we checked git blame.

We saw that that code was part of this fix and of course, it was very much needed and it wasn’t some kind of a code without purpose. We spend some time trying to figure out how that fix worked and where and how and what should we do to fix our issue. That was another moment when we got stuck.

Then we noticed another interesting thing, turns out that the bug wasn’t happening with PDF files, which means that somehow it is handled for the PDF’s. Therefore we start searching how and where in the code the PDF’s are being handled.

We found out that the case is handled for the PDF’s, so our idea was to find a way how & where to check the downloading link so that if it is on the current page we would not reset the case. We were debugging and looking for different ways. We tried to go from the opposite, but every time we would be back to the exact same function and isSecure = null. After discussing the issue and thinking what should we do next, since we coming back to the same place, we decided to comment the progress on the issue page and ask for help.

Yan replied to us really fast, his response was extremely useful.

Extremely helpful reply

However, even though we were on the right track, we kinda stuck again. We left the bug for half a day, and then Marco found this:

if (e.isMainFrame && !e.isErrorPage && !e.isFrameSrcDoc) {
if (e.url && e.url.startsWith(appConfig.noScript.twitterRedirectUrl) &&
this.props.noScript === true)
{
// This result will be canceled immediately by sitehacks, so don't
// update the load state; otherwise it will not show the security
// icon.
return
}
windowActions.onWebviewLoadStart(this.frame, e.url)
}
}

As you can see in this function we are handling the case for this issue, which is also about the icon. After looking at this function, it kicked me. Every time we were clicking on the download, the state would reset. But, if it is a PDF we are not reloading the page:

case 'reload':// Ensure that the webview thinks we're on the same location as the browser does.// This can happen for pages which don't load properly.// Some examples are basic http auth and bookmarklets.// In this case both the user display and the user think they're on this.props.location.if (this.props.tabUrl !== this.props.location &&!this.isAboutPage() &&!isTorrentViewerURL(this.props.location)) {} else if (isPDFJSURL(this.props.location)) {appActions.loadURLRequested(this.props.tabId,UrlUtil.getLocationIfPDF(this.props.location))} else {tabActions.reload(this.props.tabId)}

Which means that for the normal download link, we handle it like it is a new page. So if we would check if the link is not a new page, then don’t reset the state. Eventually:

const isNewPage = getBaseUrl(e.url) === getTargetAboutUrl('about:newtab')if (e.isMainFrame && !e.isErrorPage && !e.isFrameSrcDoc) {if (e.url && e.url.startsWith(appConfig.noScript.twitterRedirectUrl) &&this.props.noScript === true) {// This result will be canceled immediately by sitehacks, so don't// update the load state; otherwise it will not show the security// icon.return} else if (!isNewPage) {// If not new page, don't reset the state// Fixes a security icon when downloading an itemreturn}windowActions.onWebviewLoadStart(this.frame, e.url)}}

We don’t reset the state! Bingo! It was the right place. We knew that we need to check if we are on the current page we don’t need to reset the state. After a couple of attempts, it worked!

Result

Pull Request

Open Source Adventure

Sharing my learning experience as I go!

Margaryta Chepiga

Written by

Software Developer, known as overexcited girl who is passionate about technology

Open Source Adventure

Sharing my learning experience as I go!

Welcome to a place where words matter. On Medium, smart voices and original ideas take center stage - with no ads in sight. Watch
Follow all the topics you care about, and we’ll deliver the best stories for you to your homepage and inbox. Explore
Get unlimited access to the best stories on Medium — and support writers while you’re at it. Just $5/month. Upgrade