Closed Bug 335193 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: DOM: Navigation, defect, P1)

Product:
Core
Component:
DOM: Navigation
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 2 obsolete files)

Good catch
1.12 KB, patch
Biesinger
: review+
darin.moz
: superreview+
darin.moz
: approval-branch-1.8.1+
Details | Diff | Splinter Review 
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.
Attached patch Proposed patch (obsolete) — — Splinter Review
Attachment #219559 - Flags: superreview?(darin)
Attachment #219559 - Flags: review?(cbiesinger)
Attachment #219559 - Flags: approval-branch-1.8.1?(darin)
Priority: -- → P1
Summary: URI fixup should not assume all NS_NewURI failures mean malformed URIs → [FIX]URI fixup should not assume all NS_NewURI failures mean malformed URIs
Target Milestone: --- → mozilla1.9alpha
Attached patch Patch I meant to attach (obsolete) — — Splinter Review
Attachment #219559 - Attachment is obsolete: true
Attachment #219564 - Flags: superreview?(darin)
Attachment #219564 - Flags: review?(cbiesinger)
Attachment #219559 - Flags: superreview?(darin)
Attachment #219559 - Flags: review?(cbiesinger)
Attachment #219559 - Flags: approval-branch-1.8.1?(darin)
Attachment #219564 - Flags: approval-branch-1.8.1?(darin)
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?
Attached patch Good catch — — Splinter Review
Attachment #219564 - Attachment is obsolete: true
Attachment #219638 - Flags: superreview?(darin)
Attachment #219638 - Flags: review?(cbiesinger)
Attachment #219638 - Flags: approval-branch-1.8.1?(darin)
Attachment #219564 - Flags: superreview?(darin)
Attachment #219564 - Flags: review?(cbiesinger)
Attachment #219564 - Flags: approval-branch-1.8.1?(darin)
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?
Attachment #219638 - Flags: superreview?(darin)
Attachment #219638 - Flags: superreview+
Attachment #219638 - Flags: approval-branch-1.8.1?(darin)
Attachment #219638 - Flags: approval-branch-1.8.1+
Attachment #219638 - Flags: review?(cbiesinger) → review+
That call is just used to decide whether to do Google searches.
Fixed, trunk and 1.8 branch.
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.