Currently, for certain clicks on links, urlSecurityCheck is called more than once, this should not be necessary.
Created attachment 132514 [details] [diff] [review] Initial Patch v0.4d This initial patch removes the call to urlSecurityCheck from the beginning of handleLinkClick function and adds new calls futher within the function where urlSecurityCheck would have no longer been called because of the removal. I'm not sure that we do actually need a call to urlSecurityCheck for the right click (event.button=2) but I have it in there for the moment.
Created attachment 133087 [details] [diff] [review] Patch v0.4e - removes duplicate calls to urlSecurityCheck This patch removes the first call to urlSecurityCheck in handleLinkClick and adds new calls where a call to an |openNew| function returns false, an |openNew| function is not called and wrt saveModifier where handleLinkClick is called with linkNode not being null.
Comment on attachment 133087 [details] [diff] [review] Patch v0.4e - removes duplicate calls to urlSecurityCheck This really needs review from caillon.... Why the "if (linkNode)" check, by the way?
Well I'm pretty sure that I don't need the check when the linkNode is null but I'm presuming that extensions could call this with linkNode not being null otherwise why do we still take the linkNode argument in handleLinkClick? Mozilla itself only calls it with a null for that argument.
My question is, why does it matter whether linkNode is null? The URL you are testing is not associated with linkNode at this point.... And yes, linkNode used to be used and then someone tweaked something, I guess.
Eek, why was I not cc'd?
Comment on attachment 133087 [details] [diff] [review] Patch v0.4e - removes duplicate calls to urlSecurityCheck There's just something about this patch that I don't like. Maybe it's the fact that now there are so many seemingly random security checks. Some callers have them, others don't, etc. Would it make more sense to require the callers of anything that loads a URL to do the check before calling the handleLinkClick/doOpenNewFoo methods? Also note that the security check is relatively cheap, so it doesn't really hurt anything here. The performance impact of doing perhaps an extra security check is negligible, and this bug may not be worth fixing. It's better to do too many checks than too few....
Created attachment 133892 [details] [diff] [review] Patch v0.4f - reworks contentAreaClick function removing duplicate calls to urlSecurityCheck Patch reworks contentAreaClick function by inling handleLinkClick function and moving call to urlSecurityCheck into openNewTabOrWindow function where it hasn't previously been called from openNew* functions. Need to fix this so that bug 84128 can be fixed as multiple calls to urlSecurityCheck would cause multiple user alerts.
Comment on attachment 133087 [details] [diff] [review] Patch v0.4e - removes duplicate calls to urlSecurityCheck Cancelling previous request
Created attachment 163169 [details] [diff] [review] Patch v0.5 - makes sure urlSecurityCheck is called This patch: * Fixes the duplicate calls to urlSecurityCheck in handleLinkClick - instead of checking at start of function only checks if openNewWindowWith or openNewTabWith have not been called. * Fixes the fact that urlSecurityCheck is not called before trying to load the url in middleMousePaste function if ctrl key is not being pressed
I guess firefox might need a similar middleMousePaste fix.
Comment on attachment 163169 [details] [diff] [review] Patch v0.5 - makes sure urlSecurityCheck is called This is no longer anything like the current code.
This seems to have effectively been fixed when this function was rewritten at some point.