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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
Networking
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({assertion})

Trunk
mozilla1.9.3a1
assertion
Points:
---

Firefox Tracking Flags

(status1.9.2 beta2-fixed, status1.9.1 .6-fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 408159 [details] [diff] [review]
Patch

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)
Created attachment 408160 [details]
sessionstore.js that will reproduce the assertion
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
Created attachment 408494 [details] [diff] [review]
Serialize to/from nsISupports even if serializing to nsIURI would still work, plus test
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
Last Resolved: 8 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?

Updated

8 years ago
Attachment #408494 - Flags: approval1.9.2? → approval1.9.2+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4a85d4892358
status1.9.2: --- → final-fixed
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+
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c99ec76a408f
status1.9.1: --- → .6-fixed
You need to log in before you can comment on or make changes to this bug.