Closed Bug 524245 Opened 15 years ago Closed 15 years ago

Correctly serialize and deserialize nsNestedAboutURI::mBaseURI as the same interface, not as different ones

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta2-fixed
status1.9.1 --- .6-fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

(Keywords: assertion)

Attachments

(2 files, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
In the case of nsStandardURL, the nsISupports pointer is not the nsIURI pointer, which causes assertions underneath getter_AddRefs.  For some amount of backwards compatibility, this suggests reading in the pointer as nsISupports and then doing a post-QI into mBaseURI, just to be safe.

I encountered this due to a certain configuration of sessionstore.js; I'll attach that file in a second.
Attachment #408159 - Flags: review?(bzbarsky)
Um... no.  You want to either write as nsIURI and read as nsIURI or write as nsISupports and read as nsIURI.

This patch is writing as nsIURI and reading as nsISupports....  It would also assert.
Summary: nsNestedAboutURI::Write serializes mBaseURI as nsISupports, not as nsIURI → Correctly serialize and deserialize nsNestedAboutURI::mBaseURI as the same interface, not as different ones
Attachment #408159 - Attachment is obsolete: true
Attachment #408494 - Flags: review?(bzbarsky)
Attachment #408159 - Flags: review?(bzbarsky)
Comment on attachment 408494 [details] [diff] [review]
Serialize to/from nsISupports even if serializing to nsIURI would still work, plus test

Looks great.  Thanks!
Attachment #408494 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/mozilla-central/rev/360e66424053

It would be pretty safe to take this on branches, in addition to trunk; the assertion that fired isn't a very happy assertion.  Is it necessary to do so?  There was no sign anything was wrong except the assertion, so I'm not going to push it, but I wouldn't object to it.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
I think we should take this on branches, yes.
Comment on attachment 408494 [details] [diff] [review]
Serialize to/from nsISupports even if serializing to nsIURI would still work, plus test

Applies cleanly to 191/192 branches, small and simple, fixes an assertion that suggests future type errors (crashes, in C++) are possible, although none have been seen yet presumably because the pointer is generally used only through nsISupports (not nsIURI), which shouldn't fall over.  Definitely good hygiene in any case...
Attachment #408494 - Flags: approval1.9.2?
Attachment #408494 - Flags: approval1.9.1.5?
Attachment #408494 - Flags: approval1.9.2? → approval1.9.2+
Comment on attachment 408494 [details] [diff] [review]
Serialize to/from nsISupports even if serializing to nsIURI would still work, plus test

Approved for 1.9.1.6, a=dveditz for release-drivers
Attachment #408494 - Flags: approval1.9.1.6? → approval1.9.1.6+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: