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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

P1
major
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({fixed1.8.1})

Trunk
mozilla1.9alpha1
x86
Linux
fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 219559 [details] [diff] [review]
Proposed patch
Attachment #219559 - Flags: superreview?(darin)
Attachment #219559 - Flags: review?(cbiesinger)
Attachment #219559 - Flags: approval-branch-1.8.1?(darin)
(Assignee)

Updated

13 years ago
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
(Assignee)

Comment 2

13 years ago
Created attachment 219564 [details] [diff] [review]
Patch I meant to attach
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)
(Assignee)

Updated

13 years ago
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?
(Assignee)

Comment 4

13 years ago
Created attachment 219638 [details] [diff] [review]
Good catch
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 5

13 years ago
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+
(Assignee)

Comment 6

13 years ago
That call is just used to decide whether to do Google searches.
(Assignee)

Comment 7

13 years ago
Fixed, trunk and 1.8 branch.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.