335193 - [FIX]URI fixup should not assume all NS_NewURI failures mean malformed URIs

Status: RESOLVED FIXED

Description

[Boris Zbarsky [:bzbarsky, bz on IRC]]

STEPS TO REPRODUCE:

1) Change nsViewSourceProtocol::NewURI to always throw
2) View source of any page

EXPECTED RESULTS:  Error message or error page or something

ACTUAL RESULTS:  Random web page loads in view-source window and starts sprouting popups.

DETAILS:  The page is what you get for "http://view-source".  The problem is that URI fixup assumes any NS_NewURI failure means it needs to do fixup.  It should really only be doing fixup if NS_NewURI returns NS_ERROR_MALFORMED_URI, imo.

Comment 1

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Created attachment 219559 [details] [diff] [review]
Proposed patch

Comment 2

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Created attachment 219564 [details] [diff] [review]
Patch I meant to attach

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 219564 [details] [diff] [review]
Patch I meant to attach

+    if (!*aURI && rv != NS_ERROR_MALFORMED_URI) {
+        return rv;
+    }

Hm... I'm not sure this is right. in the case where the NS_NewURI function above wasn't reached, couldn't uri be null but rv be success? why not move this inside the if?

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Created attachment 219638 [details] [diff] [review]
Good catch

Comment 5

[Darin Fisher]

Comment on attachment 219638 [details] [diff] [review]
Good catch

Didn't we add a call to NS_NewURI prior to calling into CreateFixupURI?  Does that need to change?

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

That call is just used to decide whether to do Google searches.

Comment 7

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Fixed, trunk and 1.8 branch.