563235 - nsIDocument's IID and nsIDOMWindowUtils_1_9_2 were changed on a release branch

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

...and it broke various binary extensions

Comment 2

[LegNeato]

It doesn't look to me like this landed on GECKO1924_20100413_RELBRANCH, correct? If that's the case then we'll need it fixed on mozilla-1.9.2 default...but it won't hold up the build. Of course, I could be mistaken as to where it has landed.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problematic revision on 1.9.2 is this one:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5af70883c1de

This IID change affects the following branches and tags in mozilla-1.9.2:

$ hg branches | cut -d" " -f1 | while read TAG; do if hg cat -r$TAG content/base/public/nsIDocument.h | grep -A1 "define NS_IDOCUMENT_IID" | grep "0x3e162" > /dev/null; then echo $TAG; fi; done
default
COMM1925_20100427_RELBRANCH
COMM1925_20100422_RELBRANCH
GECKO1925pre_20100414_RELBRANCH

$ hg tags | cut -d" " -f1 | while read TAG; do if hg cat -r$TAG content/base/public/nsIDocument.h | grep -A1 "define NS_IDOCUMENT_IID" | grep "0x3e162" > /dev/null; then echo $TAG; fi; done
tip
THUNDERBIRD_3_1_b2_RELEASE
THUNDERBIRD_3_1_b2_BUILD2
THUNDERBIRD_3_1_b2_BUILD1
CALENDAR_1_0b2_RELEASE
CALENDAR_1_0b2_BUILD1
FENNEC_1_1b1_RELEASE
FENNEC_1_1b1_BUILD2
FENNEC_1_1b1_BUILD1

Comment 4

[LegNeato]

Thanks for checking on that. I also pulled a copy of the branch and spot checked. Looks like GECKO1924_20100413_RELBRANCH is ok. This should block 1.9.2.5+ though.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #2)
> It doesn't look to me like this landed on GECKO1924_20100413_RELBRANCH,
> correct? If that's the case then we'll need it fixed on mozilla-1.9.2
> default...but it won't hold up the build. Of course, I could be mistaken as to
> where it has landed.

Yeah, AFAIR that is correct. We waited for tagging 1.9.2.4 before landing some changes needed for fennec. Thanks dbaron for double checking.


nsIDocument_1_9_2 already exists, so the fix is straightforward. I have a question though: does the ordering of functions on the .h implementation matter for binary compatibility? I'm asking this because bug 489127 added the function NodesFromRectHelper in the middle of nsDocument.h (to keep it nearby related functions). I don't know if I should keep it there or move it to the end of the public implemented functions.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Finkle: you'll want to make sure you get this change on Fennec 1.1 as well, I suspect!

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Standard8: just so you're aware!

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to comment #5)
> nsIDocument_1_9_2 already exists, so the fix is straightforward.

No, no, no!

If nsIDocument_1_9_2 already exists and we have shipped it with a release then reusing that one leads to exactly the same potential problems (with perhaps less likelihood of encountering them). Once we've shipped an interface you have to create another one if you're changing it. You could go sequential and add _1 or add a _5 to indicate when it was added (which might help if someone else wants to change it in the same 1.9.2.5 cycle).

Bug 489127 also changed the shipped nsIDOMWindowUtils_1_9_2 interface -- we need to back that change out and create an additional interface there, too.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #8)
> (In reply to comment #5)
> > nsIDocument_1_9_2 already exists, so the fix is straightforward.
> 
> No, no, no!
> 
> If nsIDocument_1_9_2 already exists and we have shipped it with a release then
> reusing that one leads to exactly the same potential problems (with perhaps
> less likelihood of encountering them). Once we've shipped an interface you have
> to create another one if you're changing it. You could go sequential and add _1
> or add a _5 to indicate when it was added (which might help if someone else
> wants to change it in the same 1.9.2.5 cycle).
> 
> Bug 489127 also changed the shipped nsIDOMWindowUtils_1_9_2 interface -- we
> need to back that change out and create an additional interface there, too.


Ok. Also check bug 544765. nsIDOMWindowUtils_1_9_2 was changed there too. It'll need to be backed out too, and perhaps these two changes can be put together on a new interface.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Would it be better here (for review purposes and landing purposes) to create a patch that fix the changes (revert the values to the old ones and add the new interfaces), or back out everything and create a new full patch with the changes in place?

Also, I have this pending question: does the ordering of functions on the .h implementation matter for binary compatibility?
I'm assuming it does and I'm moving the function declaration to the end of the public functions on nsDocument.h

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #10)
> Also, I have this pending question: does the ordering of functions on the .h
> implementation matter for binary compatibility?
> I'm assuming it does and I'm moving the function declaration to the end of the
> public functions on nsDocument.h

It doesn't matter.  The binary compatibility issue is preserving (1) the IID and (2) the ns*I*Document vtable.  Reordering things in nsDocument *might* (or might not) affect people who have nsDocument pointers, but we don't care about them; we only care about callers who have ns*I*Document pointers.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch from 489127 keeping current interfaces unchanged

So I backed out bug 489127 (http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5af70883c1de) locally, and recreated a full patch for it, creating the interfaces nsIDocument_MOZILLA_1_9_2_5_BRANCH and nsIDOMWindowUtils_1_9_2_5 and adding the new functions to said interfaces.


I don't know what should be the process for this, so I'm setting the review and superreview flags and hopefully someone can pick it. Please take a careful look on the implementation of the new interfaces because that's where this patch has differences from what landed.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 443283 [details] [diff] [review]
Patch from 489127 keeping current interfaces unchanged

It's better to pick people for review? and superreview? requests.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Felipe, could you provide a patch which can be applied to 1.9.2 tree without
backing out anything; so just the new interfaces and some changes to the code
which uses those interfaces. That would be easier to review, at least for
me since I didn't review the original patch.

Comment 15

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #14)
> Felipe, could you provide a patch which can be applied to 1.9.2 tree without
> backing out anything; so just the new interfaces and some changes to the code
> which uses those interfaces. That would be easier to review, at least for
> me since I didn't review the original patch.

Sure

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Attachment: Revert IIDs and create new interfaces

This patch reverts the IID changes to nsIDocument and nsIDOMWindowUtils_1_9_2 and moves the functions added to the new interfaces _1_9_2_5.
(I didn't reorder the function in nsDocument.h per comment 11).

I also moved the things added in bug 544765 from nsIDOMWindowUtils_1_9_2 to 1_9_2_5, so the interface is reverted to the original form in a single step.

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 443399 [details] [diff] [review]
Revert IIDs and create new interfaces

Thanks for fixing this.

sr=dveditz

Comment 18

[Mark Banner (:standard8)]

Don't patches on blocking bugs still require approval for checkin on branch?

Comment 19

[Reed Loden [:reed]]

(In reply to comment #18)
> Don't patches on blocking bugs still require approval for checkin on branch?

Indeed.

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 443399 [details] [diff] [review]
Revert IIDs and create new interfaces

Ok, I did not know that

Comment 21

[Daniel Veditz [:dveditz]]

Before checking in it's a good idea to check tinderbox (not tinderboxpushlog). It'll give the current state and rules for that branch--which most of the time is https://wiki.mozilla.org/TreeStatus#Rules

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 443399 [details] [diff] [review]
Revert IIDs and create new interfaces

Approved for 1.9.2.5, a=dveditz for release-drivers

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Attachment: Revert IIDs and create new interfaces

I was testing if the patch still applies cleanly, and saw that the nsIDOMWindowUtils_1_9_2 uuid was changed again on branch in the meantime. This patch also reverts that change.
The change from the reviewed patch is 1 line: only the uuid removed changes.

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 452888 [details] [diff] [review]
Revert IIDs and create new interfaces

Looks good, thanks!
r=dveditz
Approved for 1.9.2.6, a=dveditz

Comment 25

[Dietrich Ayala (:dietrich)]

pushed: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8ac0dfc2a34a