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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: sessionstore.js that will reproduce the assertion

Comment 2

[Boris Zbarsky [: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.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Serialize to/from nsISupports even if serializing to nsIURI would still work, plus test

Comment 4

[Boris Zbarsky [: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!

Comment 5

[Jeff Walden [:Waldo]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I think we should take this on branches, yes.

Comment 7

[Jeff Walden [:Waldo]]

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...

Comment 8

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4a85d4892358

Comment 9

[Daniel Veditz [:dveditz]]

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

Comment 10

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c99ec76a408f