Last Comment Bug 524245 - Correctly serialize and deserialize nsNestedAboutURI::mBaseURI as the same interface, not as different ones
: Correctly serialize and deserialize nsNestedAboutURI::mBaseURI as the same in...
Status: RESOLVED FIXED
: assertion
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.3a1
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-23 18:34 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2009-11-06 13:03 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta2-fixed
.6-fixed


Attachments
Patch (1.17 KB, patch)
2009-10-23 18:34 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
sessionstore.js that will reproduce the assertion (6.71 KB, text/plain)
2009-10-23 18:35 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details
Serialize to/from nsISupports even if serializing to nsIURI would still work, plus test (3.52 KB, patch)
2009-10-26 16:05 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bzbarsky: review+
jst: approval1.9.2+
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2009-10-23 18:34:51 PDT
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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2009-10-23 18:35:47 PDT
Created attachment 408160 [details]
sessionstore.js that will reproduce the assertion
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2009-10-23 19:03:45 PDT
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] (remove +bmo to email) 2009-10-26 16:05:16 PDT
Created attachment 408494 [details] [diff] [review]
Serialize to/from nsISupports even if serializing to nsIURI would still work, plus test
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2009-10-26 19:06:14 PDT
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] (remove +bmo to email) 2009-10-26 21:04:11 PDT
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 [:bz] (still a bit busy) 2009-10-27 11:50:45 PDT
I think we should take this on branches, yes.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2009-10-27 16:22:09 PDT
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] (remove +bmo to email) 2009-11-03 18:08:02 PST
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4a85d4892358
Comment 9 Daniel Veditz [:dveditz] 2009-11-04 17:13:24 PST
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] (remove +bmo to email) 2009-11-06 13:03:20 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c99ec76a408f

Note You need to log in before you can comment on or make changes to this bug.