Last Comment Bug 682845 - Bug 665706 changes IDL without changing IIDs, changes nsStandardURL serialization format
: Bug 665706 changes IDL without changing IIDs, changes nsStandardURL serializa...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: All All
: -- major (vote)
: mozilla9
Assigned To: Randell Jesup [:jesup]
:
Mentors:
: 682697 (view as bug list)
Depends on:
Blocks: 665706 682697
  Show dependency treegraph
 
Reported: 2011-08-29 08:32 PDT by Boris Zbarsky [:bz]
Modified: 2011-12-14 13:36 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Fix serializations of nsStandardURL broken by bug 665706 (4.14 KB, patch)
2011-09-01 12:22 PDT, Randell Jesup [:jesup]
bzbarsky: feedback+
Details | Diff | Review
Update IIDs, fix serializations of nsStandardURL broken by bug 665706 (7.43 KB, patch)
2011-09-09 15:37 PDT, Randell Jesup [:jesup]
bzbarsky: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2011-08-29 08:32:11 PDT
Bug 665706 changed some interfaces without changing their IIDs.  Furthermore, it changed the nsIObjectOutputStream serialization format of nsStandardURL, which will cause fastload and session restore failures.

The IIDs need to be revved.  For serialization, probably best to switch to serializing empty string and when deserializing appending whatever is read to the filepath or something...
Comment 1 Boris Zbarsky [:bz] 2011-08-29 08:33:38 PDT
This should ideally be resolved before the aurora migration for 9, due to the extension compat issues.
Comment 2 Randell Jesup [:jesup] 2011-09-01 12:22:12 PDT
Created attachment 557603 [details] [diff] [review]
Fix serializations of nsStandardURL broken by bug 665706
Comment 3 Randell Jesup [:jesup] 2011-09-01 12:38:29 PDT
Comment on attachment 557603 [details] [diff] [review]
Fix serializations of nsStandardURL broken by bug 665706

Patch for the serialization issues.  IID patch to come.

Tested with the tet profile here; loads and works.  Still need to test backwards and forwards compatibility with URLs with params.

bz: reasonable approach?  The backwards-compatibility aspect is the riskiest, in that an old browser would have the param in the filepath/etc where it wouldn't have before.  Otherwise we'll have to parse it out the old way here to write it out (and read it always that way); basically locking in the old serialization forever.

Need to figure out if this is backwards-compatibility issue is a real problem or if it's safe (or just a very unlikely annoyance).
Comment 4 Boris Zbarsky [:bz] 2011-09-02 13:01:05 PDT
Comment on attachment 557603 [details] [diff] [review]
Fix serializations of nsStandardURL broken by bug 665706

This looks fine, though some spaces after comma in the Merge() callsites would be good.
Comment 5 Randell Jesup [:jesup] 2011-09-09 15:26:07 PDT
So, I've been busy.  I believe the patch here plus IID changes, and another addition to the hack to recognize different IIDs for nsIURI, will solve this problem.

But this changes the serialization yet again, even though it's effectively going back to an old one.  The question is what's the impact of that on people running this patch in FF9 (perhaps little to one).  Admittedly, limited breakage like this may be an "allowable" sort of breakage for nightly users, and if I can ascertain that it's that case I can fix it once, and pull out the "fix-it" code later after virtually all profiles have gotten updated.  Also, we must take a fix for bug 682762 also caused by the bug 665706 change.

While it definitely breaks this testcase (in bug 682697), I'd like to really understand the impact of this on users and how likely is this serialization change (if not fixed) likely to break things.  And can we ever get rid of this compatibility code?

IID+nit patch coming
Comment 6 Randell Jesup [:jesup] 2011-09-09 15:37:17 PDT
Created attachment 559600 [details] [diff] [review]
Update IIDs, fix serializations of nsStandardURL broken by bug 665706
Comment 7 Randell Jesup [:jesup] 2011-09-09 15:43:40 PDT
Comment on attachment 559600 [details] [diff] [review]
Update IIDs, fix serializations of nsStandardURL broken by bug 665706

And we'll need SR here, unless you'd like to SR it bz and have someone else r= it (I assume we still(?) disapprove of r= and sr= from the same person if we can avoid it)
Comment 8 Boris Zbarsky [:bz] 2011-09-09 19:00:56 PDT
I would way rather have the breakage of this sort (affects only some tabs, etc) for nightly users than for everyone updating stable Firefox!
Comment 9 Boris Zbarsky [:bz] 2011-09-09 19:02:15 PDT
Comment on attachment 559600 [details] [diff] [review]
Update IIDs, fix serializations of nsStandardURL broken by bug 665706

I think that given that this is just addressing sr comments on another bug, my reviewing it is good enough...
Comment 10 Randell Jesup [:jesup] 2011-09-10 02:21:56 PDT
(In reply to Randell Jesup [:jesup] from comment #5)
> So, I've been busy.  I believe the patch here plus IID changes, and another
> addition to the hack to recognize different IIDs for nsIURI, will solve this
> problem.

No need to update the hack as this changes nsIURL and associated IIDs, not nsIURI

> Also, we must take a fix for bug 682762 also caused by the
> bug 665706 change.

We still need that.
Comment 11 Matt Brubeck (:mbrubeck) 2011-09-11 06:37:49 PDT
http://hg.mozilla.org/mozilla-central/rev/c5ed260e9a49
Comment 12 Randell Jesup [:jesup] 2011-09-15 15:27:44 PDT
*** Bug 682697 has been marked as a duplicate of this bug. ***
Comment 13 Alex Keybl [:akeybl] 2011-12-01 14:36:30 PST
If there's any extension compatibility issues, please add the "addon-compat" keyword to this bug.

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