Prevent app crash with malformed hrefs

[Jaifroid]

This problem is a bit tricky. In essence, the malformed href causes a crash on any attempt to access any property of the href. This means we have two choices: use jQuery, with its in-built error handling, or else we have to use try … catch. So I'm posting two commits here, one with jQuery and the second with try … catch. Both solve the issue. @mossroy , if you prefer the jQuery solution, or a variant of it, I'll revert the second commit. Although generally I prefer not to use jQuery when it's unnecessary, here I think it provides a tangible benefit, so I'm inclined towards the jQuery solution.

[mossroy]

The code looks good.
In your jQuery variant, why on line 890 did you replace this.href with href?
I don't remember why we used this.href, but it seems to me it was for a good reason (at least at that time), even if I don't remember the detail. So I find this change a bit risky. Apart from this, both methods (with or without jQuery) are fine for me.
I do not have the september wikimed for now, so could not test the code. But I trust you. If you'd like me to test anyway, I might do that later this week.

[Jaifroid]

Don't bother downloading the September WikiMed, because I've now discovered the crash only occurs in Edge and in UWP (and apologies for not having tested more widely before -- I was more preoccupied with fixing it). Firefox and Chromium don't have a problem with the malformed URL, even though it doesn't yield a valid URL. I think it's still worth doing the jQuery solution, because it's generic. However, I need to find out at what point we changed to this.href, and why -- it seems a bit odd that we have a variable, href, which has just been initialized with the URL using jQuery, and then not to use it. We can't use this.href in the jQuery solution, because any attempt to access the properties of this causes a crash in Edge and in UWP (which uses the Edge engine).

My suspicion is that we started to rewrite some of these functions without jQuery when we got the problem with #361 . For a short while, we started substituting pure JS in these routines, then we reverted that when we found another fix, but maybe not fully? However, I'll locate the spot in the commit history where it changed (if it's not too far in the remote past!), before proceeding.

[Jaifroid]

The use of this.href instead of $(this).attr("href") was introduced in this commit 4a933eb#diff-bca463ed8ccf5e2058dfe307a95e8501R826 . It seems to be the case that you used that syntax simply because you'd deleted var url = $(this).attr("href"); (as it was then). It doesn't look particularly significant, as my proposed simplification/revert to the already defined variable href b09959c#diff-bca463ed8ccf5e2058dfe307a95e8501R890 would not make a substantial difference except that jQuery works around the browser quirk here (probably simply by catching exceptions). Do you agree, @mossroy , or does it prompt you to remember a reason for writing the code this way? In principle, the jQuery code merely means: get the attribute "href" of the currently selected anchor. If you agree, I'll use the jQuery solution, as try...catch is ugly.

[Jaifroid]

PS Version of the historical commit without yellow highlighting: 4a933eb (line 826).

[mossroy]

I'm OK with your proposal, as long as you test it with various ZIM files (notably stackoverflow) and link types (like geo: etc)

[Jaifroid]

Will do :-)

[Jaifroid]

@mossroy : Just to let you know your memory was correct, and there is a difference between what is returned by this.href and what is returned by jQuery. The former returns, e.g. file:///C:/Users/xxxx/Source/Repos/kiwix-js/www/A/question/84/how-important-are-accents-in-written-spanish.html, while the latter returns question/84/how-important-are-accents-in-written-spanish.html for the same href. This accounts for your having made that change in a commit entitled "Handle subdirectories in article namespace", where you need the href to show the protocol.

I'm looking at the best options in the light of this. Thanks for your caution!

[mossroy]

Nice work! I just regret that I did not add a comment in the code to explain that.
Could you please add it in your PR (if this line of code is finally kept)?

[Jaifroid]

I'll definitely add a comment to explain.

Just FYI, in case you're curious, here's what I found out:

• jQuery $(this).attr('href') = JS this.getAttribute('href') -- both of these merely return the content of the href attribute of the anchor link;
• jQuery $(this).prop('href') = JS this.href -- both of these get the fully qualified URL that the browser has worked out by combining base href with the protocol and the anchor link's href.

Unfortunately, jQuery's $(this).prop syntax, which is the one we would need here, doesn't do any error checking, and the crash in Edge/UWP happens the same as with this.href, this.protocol, this.host, etc.

Therefore we're back to Option 2, using try...catch.

I'll revert the revert and test more.

[mossroy]

I did not test, but the code looks good.
Thanks for the comments, that are very necessary.

[Jaifroid]

Thanks @mossroy , I ran out of time this week, and I need to do a bit more testing of this PR in different scenarios. I aim to do that at the weekend, after which I'll squash/merge if I find no more issues with the code. It's interesting how what I thought at first would be a "little fix" turned out to have far more complications than I'd realized!