Closed Bug 267833 Opened 20 years ago Closed 17 years ago

[FIX]Fire XBL constructors from EndUpdate(), not before

Categories

(Core :: XBL, defect, P1)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.8.0.15, fixed1.8.1.8)

Attachments

(3 files, 3 obsolete files)

I've been thinking about bug 228557 and related issues some more, and the more I
think about it, the more I think the right thing to do is to not fire XBL
constructors at all from inside frame construction.

It seems to me that the right place to ProcessAttachedQueue() would be in the
outermost EndUpdate (not in nested ones).  nsBindingManager already implements
nsIDocumentObserver, so keeping track of BeginUpdate/EndUpdate calls would be
pretty simple.

This would have the following benefits:

1)  Constructors not firing during frame construction or layout
2)  Constructors not firing during a nsIDocumentObserver notification at all (so
    that if the constructor removes the node being notified on, say, other
    observers won't get confused).

We'd have to manually ProcessAttachedQueue in PresShell::InitialReflow,
probably.  But that's not so bad.

Can anyone see any obvious problems with this approach?
Blocks: 194952
Attached patch Something like this (obsolete) — Splinter Review
Attachment #172234 - Flags: superreview?(shaver)
Attachment #172234 - Flags: review?(bryner)
Priority: -- → P1
Summary: Fire XBL constructors from EndUpdate(), not before → [FIX]Fire XBL constructors from EndUpdate(), not before
Target Milestone: --- → mozilla1.8beta
Comment on attachment 172234 [details] [diff] [review]
Something like this

So beautiful. sr=shaver, please file a bug on PAQ() possibly tearing down the
document.
Which part of it? The fact that it can (because it runs random script), or the
fact that this can happen at some odd places currently?  Not much we can do
about the former...
Comment on attachment 172234 [details] [diff] [review]
Something like this

>+  // XXXbz this should really do a document observer batch or something...
>+  // Messing with the frame tree while we might still be parsing the document
>+  // is just wrong.  We can't flush out content now, though, since that can
>+  // trigger destruction of aEvent->mFrame, which we wouldn'd detect.

typo

The only potential problem I see is if some caller calls
nsIDocument::ContentAppended/Inserted/Removed without doing a Begin/EndUpdate
batch.	Then we wouldn't run ProcessAttachedQueue() at all, right?  For
example, it looks like this _may_ happen in nsHTMLContentSink, though there may
be other constraints that I'm not aware of that cause that to not be a problem.
 Also perhaps in nsXULContentBuilder::OpenContainer.
Comment on attachment 172234 [details] [diff] [review]
Something like this

> Then we wouldn't run ProcessAttachedQueue() at all, right?

That's correct.  Note that anyone doing that with aNotify set to PR_TRUE is
already rather buggy, since he'll confuse the sink, if nothing else.  The sink
itself does look like it has issues, and you're right about the content
builder.  I'll try to figure out what that code is doing.  :(
Attachment #172234 - Flags: superreview?(shaver)
Attachment #172234 - Flags: superreview-
Attachment #172234 - Flags: review?(bryner)
Attachment #172234 - Flags: review-
Summary: [FIX]Fire XBL constructors from EndUpdate(), not before → Fire XBL constructors from EndUpdate(), not before
Target Milestone: mozilla1.8beta → mozilla1.9alpha
Blocks: 281657
Blocks: 285732
Blocks: 287981
Blocks: 330563
Blocks: 330445
Depends on: 332807
Blocks: 332807
No longer depends on: 332807
Blocks: 336960
Blocks: 344434
Blocks: 354645
So sicking fixed the issues in the sink.  Neil, thoughts on the content builder?  I still think we should do this for 1.9, but I'm pertty sure I won't have time to sort out all the builder stuff...
Flags: blocking1.9?
No, the sink still doesn't call Begin/EndUpdate, I just fixed nsGenericDOMDataNode.
Hmm...  I thought I'd reviewed a patch where you added begin/end update calls to or around FlushTags...
Blocks: 363280
Hey Bz/Sicking - are we still planning on this for 1.9?
We should try to fix this for 1.9 yes. Bz, let me know if you want me to take it on.

The sink *is* fixed now, but note that InsertChildAt/RemoveChildAt/SetAttr etc also calls Begin/EndUpdate.
If aNotify is true we already call Begin/EndUpdate.  And if aNotify is false, we won't construct frames under InsertChildAt/etc.

So the only case to worry about are people who manually call ContentAppended/ContentInserted.  At the moment that's the sinks and the content builder.  I think we should just make sure those call begin/end update... and perhaps add some asserts to catch others who mess it up.  I'll see what I can do here.
And can I say.... _wow_ that patch is against old code.  ;)
Attached patch Proposed fix (obsolete) — Splinter Review
This is a lot more careful than the first patch...

Neil, any suggestions for testing the template builder stuff?
Attachment #172234 - Attachment is obsolete: true
Attachment #253827 - Flags: superreview?(jonas)
Attachment #253827 - Flags: review?(enndeakin)
Summary: Fire XBL constructors from EndUpdate(), not before → [FIX]Fire XBL constructors from EndUpdate(), not before
There's a pile of automated template tests in bug 368097. 

I don't have any automated tests that check opening containers, but the Thunderbird account settings dialog should be suitable as a manual test. The list of accounts should work properly when adding new accounts and opening/closing the tree rows.
Excellent.  This patch passes those tests to the same extent that an unpatched build does.

Thunderbird's account manager crashes, however.  I'll need to figure out whether it's due to this patch and if so why.
Comment on attachment 253827 [details] [diff] [review]
Proposed fix

The changes here look ok to me, although I'm not familiar enough with the content sink stuff.
Attachment #253827 - Flags: review?(enndeakin) → review+
Yeah, I figure sicking will review that.  :)
I'm a bit worried about some of the places where we call PAQ, such as for PresShell::ContentRemoved and ProcessPendingRestyles. Those seem like unsafe places to run script?

Can you file a bug on the nsCSSFrameConstructor::CreateListBoxContent thing?

Put the RemoveBlocker call after the PAQ call. Just feels better. Let me know if you think otherwise.
> I'm a bit worried about some of the places where we call PAQ

I pushed things up as far as I could before running into what are basically external APIs exposed on nsIPresShell...  In order:

We need to do _something_ either in InitialReflow or in its callers.  I don't like it either, but then again I don't like InitialReflow all that much... We should maybe file a followup to see how we can make it better.

RecreateFramesFor() is a "public" layout API.  At the moment, all the callers are in content code, so refcounted.  So it should be reasonably safe to run script there.

We don't call PAQ in PresShell::ContentRemoved.  That call is actually in PresShell::ReconstructFrames.  Which only has one caller, and that caller is running directly under the event loop.  So that's actually about as safe as it gets.

PresShell::Observe I sort of _hope_ is safe....

ProcessPendingRestyles is a little more worrisome.  But at the moment there are only two callers into it: the restyle event and FlushPendingNotifications.  The former is safe, since it's right under the event loop.  I'm not happy with the FlushPendingNotifications, but I'm not sure how else we can do there.

> Can you file a bug on the nsCSSFrameConstructor::CreateListBoxContent thing?

Will do.

> Put the RemoveBlocker call after the PAQ call.

Again, will do.
Flags: blocking1.9? → blocking1.9+
The call site to RecreateFramesFor in nsSVGUseElement doesn't seem safe. I wonder if we could make RecreateFramesFor call PAQ asyncronously, and in the places where we really need the bindings o be there syncronously (if any of them really do) call PAQ manually there? Is that something we can do for PresShell::Observe too?

The callsite to RecreateFramesFor in nsGenericHTMLElement seems like dead code, so that should be safe :)

FlushPendingNotifications needs to die :(
How can it die? Anywhere that depends on having an up-to-date layout needs to be able to ensure that somehow. Those places just need to be able to deal with arbitrary script execution as well.
> The call site to RecreateFramesFor in nsSVGUseElement doesn't seem safe.

Right you are.  :(

> I wonder if we could make RecreateFramesFor call PAQ asyncronously

Hmm.  That might be doable.  Even more interestingly, could we add a boolean to RecreateFramesFor to indicate whether the recreate should be synchronous or not?   I don't know about SVG, but I think both XBL callsites could do it async.  nsObjectLoadingContent needs it to be sync.  Editor could be async.

But yeah, I think all of these could run the constructors async; most of them should never have any constructors anyway, or are coming from async code in the first place (the XBL loading).  So maybe we should just do that and not worry about it too much.

> The callsite to RecreateFramesFor in nsGenericHTMLElement seems like dead 
> code,

Yep.  I'll just remove it.  ;)
One thing that I thought of that would be really cool is if we make the bindingmanager automatically post a AsyncPAQ event every time a binding is added, unless one already is posted. Then in the few places where we really need to ensure that constructors has run make a manual call to PAQ.

Roc: I'll file a separate bug on the Flush thing, I do have a plan for it.
> is if we make the bindingmanager automatically post a AsyncPAQ event

Hmm.  That would be _really_ nice, yeah.  Want me to go ahead and do that?
If you've got the cycles it'd be great if you could give it a try.
Btw, it might still be a good idea to call PAQ on the last EndUpdate call so that inserted DOM elements get their bindings properly processed before the mutation function returns.
Yeah, indeed.  That would be the plan.

I'll try to give this a show sometime this week.
Attached patch With that change (obsolete) — Splinter Review
The content sink and template changes may not strictly speaking be needed now, but I think they're still the right thing to do.  I took out some of the other PAQ calls.
Attachment #253827 - Attachment is obsolete: true
Attachment #257403 - Flags: superreview?(jonas)
Attachment #257403 - Flags: review?(jonas)
Attachment #253827 - Flags: superreview?(jonas)
Comment on attachment 257403 [details] [diff] [review]
With that change

This looks great! I hope that this works fairly well. I'm sure we're going to have to add explicit PAQ calls in a few more places, but we should do that as we find issues.

>Index: layout/base/nsCSSFrameConstructor.cpp
>@@ -12203,16 +12229,18 @@ nsCSSFrameConstructor::CreateListBoxCont
>                                 aParentFrame, aChild->Tag(),
>                                 aChild->GetNameSpaceID(),
>                                 styleContext, frameItems, PR_FALSE);
>     
>     nsIFrame* newFrame = frameItems.childList;
>     *aNewFrame = newFrame;
> 
>     if (NS_SUCCEEDED(rv) && (nsnull != newFrame)) {
>+      // XXXbz this call could tear down the document....  There
>+      // really needs to be a better way to do this.
>       mDocument->BindingManager()->ProcessAttachedQueue();

Lets try to nuke this and see if that works.

Rather than adding a DoPAQ, you could simply make PAQ cancel the current event if one is posted and null out the member.

Also, could you rather than null out the member and always recreate the runnable, simply reuse the same one multiple times and have a flag for if the runnable is currently in the queue?

r/sr=me with those changes.
Attachment #257403 - Flags: superreview?(jonas)
Attachment #257403 - Flags: superreview+
Attachment #257403 - Flags: review?(jonas)
Attachment #257403 - Flags: review+
Actually, it might be better to not cancel the runnable in PAQ if it's already posted. Otherwise, if someone adds one or two bindings at a time and manually calls PAQ, we might end up posting and canceling lots of runnables.

Feel free to go either way
> Lets try to nuke this and see if that works.

Seems to.  Let's try it!

> simply reuse the same one multiple times

I thought about this.  It's hard to do this without leaking.  If you want, I'm happy to try to make nsRunnableMethod participate in cycle collection... in a separate bug.  Without that, we leak.
Attachment #257403 - Attachment is obsolete: true
I tried checking this in, and Txul jumped a good bit.  I backed out.

I don't know whether it's a real jump, since it jumped to about the level it was at before bug 372729 happened.  So I think I'd like the fix for bug 372729 to land first, then try relanding this again.  If it still jumps, I'll try profiling, I guess....
Depends on: 372729
jonas suggested that taking a perf hit might be required to fix this, but bz's plan to investigate to understand what's going on sounds like a good idea.

other thoughts on this?
If this in fact causes Tp regressions, it is possible that those can be fixed by adding synchronous PAQ calls somewhere. Profiling is needed to figure out where though.
And I can't seem to reproduce the Txul changes locally...  Raw data from Txul on my machine (note that the profiler was NOT running):

BEFORE PATCH:

bin% proffox -chrome "file:///home/bzbarsky/mozilla/profile/mozilla/xpfe/test/winopen.xul" 
Jprof: Initialized signal handler and set timer for 8192 Hz, 0 s initial delay
openingTimes=1055,885,877,910,1201,882,932,1211,923
avgOpenTime:986
minOpenTime:877
maxOpenTime:1211
medOpenTime:923
__xulWinOpenTime:923

bin% proffox -chrome "file:///home/bzbarsky/mozilla/profile/mozilla/xpfe/test/winopen.xul" 
Jprof: Initialized signal handler and set timer for 8192 Hz, 0 s initial delay
openingTimes=1240,913,919,865,910,881,1589,880,873
avgOpenTime:1008
minOpenTime:865
maxOpenTime:1589
medOpenTime:910
__xulWinOpenTime:910

AFTER PATCH:

bin% proffox -chrome "file:///home/bzbarsky/mozilla/profile/mozilla/xpfe/test/winopen.xul" 
Jprof: Initialized signal handler and set timer for 8192 Hz, 0 s initial delay
openingTimes=1038,878,948,877,887,900,1025,1246,942
avgOpenTime:971
minOpenTime:877
maxOpenTime:1246
medOpenTime:942
__xulWinOpenTime:942

Jprof: Initialized signal handler and set timer for 8192 Hz, 0 s initial delay
openingTimes=1017,890,938,894,886,1188,910,913,1212
avgOpenTime:983
minOpenTime:886
maxOpenTime:1212
medOpenTime:913
__xulWinOpenTime:913

No significant difference in the actual times...
So I checked this in again, since the bug 363253 Txul drop still seems bogus to me.  If it turns out that this is a real jump, I'll try to see what I can do, I guess...

Fixed, but we still need to write some tests here.  Help very much wanted with that; I don't actually know XBL well enough to write tests quickly.  :(
Status: NEW → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Would be great if someone could go through the bugs marked as blocked by this one and see which ones were fixed by this checkin.

FWIW, i think wrt testing this, I think problems will be more likely in things that currently use XBL, like XUL, than in testcases written specifically for this bug.
> I think problems will be more likely in things that currently use XBL

Of course.  But we should add some general tests like "if I clone a node with a binding, the constructor fires" and "If I insert a node into the DOM, the constructor fires before the appendChild/insertBefore call returns".  That sort of thing.  Making sure that in the obvious cases when the constructor should fire, it does.

For testing that it doesn't fire when it shouldn't, we can use the tests in the various dependencies...
Depends on: 373727
landing this seems to have caused bug 373719, wherein the places tree's view gets nullified during what seems to be a reflow pass after the constructor fires.
Depends on: 373719
Depends on: 373721
Depends on: 373756
for the places regression (you need --enable-places-bookmarks to see it) see bug #373944 for the workaround.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
This is nominated for 1.8.1.5 but we're concerned about the regressions. Did the places regression get fixed, or was it "fixed" by changing the places code? If the latter we could still need to worry about breaking extension compatibility.

Were any extensions known to have been broken by the change?

(This is desired for the 1.8.1 branch because it's the basis for a fix for a couple of other bugs with security implications)
This did cause bug 373719, but afaik that's the only one. There might be other regressions on branch though since that's pretty different at this point.

Seth, can the fix for bug 373719 get ported to branch? Do you have any feeling for if there will be other issues on branch?
I believe the fix for bug 373756 also fixes the places regression.  I'd love to have confirmation, of course.

I should have nominated it for branches do; done that now.
Blocks: 360599
jonas / bz / dveditz:  sorry for the slow response time.

Is the open question:  is it safe to land this fix and the fix for bug #373756 on the branch?  I'm not sure about extensions or other places in the code, but here's what I can do to help you guys answer that question:

now that #373756 is fixed, I'll back out all the four workarounds we made before bz fixed #373756 locally and make sure everything still works.  the backout is tracked by bug #373944.

Once I do that, we'll have some confirmation of what boris asks in comment #44:  "I believe the fix for bug 373756 also fixes the places regression.  I'd love to have confirmation, of course."

I should have an answer later tonight.
Yup, that is the question. We clearly can't know if we'll break no extensions, but the more workarounds we can back out in our tree, the more confident we can be that extensions won't break.
I backed out bug #373756 out of my local tree, and backed out the places workarounds (see bug #373944 for the exact patch), rebuilt the whole thing and I am not able to reproduce any of the problems.

specifically, I'm looking for problems with any tree that has the place attribute specified:

mozilla\browser\components\places\content\bookmarkProperties.xul(157):        place="place:folder=2&group=3&excludeItems=1&excludeQueries=1&excludeReadOnlyFolders=1"
 
mozilla\browser\components\places\content\bookmarksPanel.xul(73):        place="place:folder=2&queryType=1"
  
mozilla\browser\components\places\content\moveBookmarks.xul(72):            place="place:folder=2&group=3&excludeItems=1&excludeReadOnlyFolders=1" 
mozilla\browser\components\places\content\places.xul(338):          place="place:folder=2&group=3&excludeItems=1&queryType=1"
  
mozilla\browser\components\preferences\selectBookmark.xul(28):        place="place:folder=2&group=3&excludeQueries=1"

so at this point, I can't confirm that bug #373756 fixes the places regression, because even without that fix, I don't see the places problems anymore.

mano / bz, do you want to check mac and linux?
> mano / bz, do you want to check mac and linux?

I've checked in the change to backout the places changes to work around this bug, so all you need to do (after updating) is back out the fix for bug #373756.

to answer a question from jonas in comment #43, "Seth, can the fix for bug 373719 get ported to branch?".  The 1.8 branch isn't places based, so we wouldn't backport the work around.
> I've checked in the change to backout the places changes to work around this
> bug.

I've backed out that checkin as it caused an unexpected performance regression, see bug #385208
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
Too close to 2.0.0.5 code-freeze to take given the number of regressions.
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
Actually, pretty much all of those were the same regression (or rather fixed by a single patch).  But yeah, I'd be happy to land this early in the next cycle.
Attachment #257535 - Flags: approval1.8.1.6?
Blocks: 387844
Flags: blocking1.8.0.13+ → blocking1.8.0.14?
Depends on: 394676
Comment on attachment 257535 [details] [diff] [review]
Updated to comments

approved for 1.8.1.7, a=dveditz for release-drivers

Please land soon, the more testing the better.
Attachment #257535 - Flags: approval1.8.1.7? → approval1.8.1.7+
Actually, a new regression was found, bug 394676.

Boris, would be great to get bug 394014 in the alpha so that we can land this on branch sooner.
Blocks: 228557
Attached patch Merged to branchSplinter Review
Merging this in kinda sucked.  It should really get another review.  :(

The patch includes the followup fixes for bug 373756, bug 394676.  It also includes the fix for bug 394014, which is more or less a followup as well.

The one major difference between this patch and trunk is that we're not doing update batches in the HTML sink on branch, so the binding ctors there will all be async off the event.  I think that should be fine, since there's not much sync XBL going on in HTML documents anyway.
Attachment #280936 - Flags: superreview?(jonas)
Attachment #280936 - Flags: review?(jonas)
Attachment #280936 - Flags: approval1.8.1.8?
Attachment #280936 - Flags: approval1.8.0.14?
bz, do you expect some xbl functionality to cause use after free after this patch?
Uh... what do you mean, exactly?  The idea of this patch is to remove a number of situations of the sort you describe.  Are you asking whether it removes all the known ones?
(In reply to comment #56)
> Uh... what do you mean, exactly?  The idea of this patch is to remove a number
> of situations of the sort you describe.  Are you asking whether it removes all
> the known ones?
> 

i am more interested in unknown ones.
i was asking if some expected misuse of xbl may cause similar troubles. 
So how come we don't need the BeginUpdate/EndUpdate calls from the sink? Don't we need the EndUpdate call there to ensure that we call PAQ as needed? Looks good otherwise, but wanted that explained before marking r+
We'd need begin/end if we wanted to PAQ right there and then. As it is, it'll happen off the event.

We really needed those before the event thing got added, but with it we're a lot more robust in terms of what we can handle.
Hmm.. what's the downside of adding it? I'm mostly scared about doing something different on branch than trunk here.
The main downside is when I started merging I discovered that it had landed as part of the <script> blocking/unblocking thing, but that that part didn't land on branch...  And then there were some followups on trunk because there was unsafe begin/end going on there, etc.  So basically, main downside is pulling in a whole bunch of changes, having to merge them in, and not being really sure how they'll behave on branch.

Since I doubt many non-XUL sites expect sync XBL anything, much less ctors (for example, because such XBL loads aren't sync anyway), I think it's safer to just not add this at all.
Comment on attachment 280936 [details] [diff] [review]
Merged to branch

cool! r/sr=me
Attachment #280936 - Flags: superreview?(jonas)
Attachment #280936 - Flags: superreview+
Attachment #280936 - Flags: review?(jonas)
Attachment #280936 - Flags: review+
Attachment #257535 - Flags: approval1.8.1.8+
I'm worried about a couple of the regressions from this one. Bug 373719 was basically not fixed, it was worked around in places; does that mean some add-ons might also break?

Bug 373719 was fixed by the patch in bug 373756 as far as I know.  See bug 373944 (which hasn't landed yet for performance reasons that are completely unrelated to this bug).
Comment on attachment 280936 [details] [diff] [review]
Merged to branch

approved for 1.8.1.8, a=dveditz for release-drivers
Attachment #280936 - Flags: approval1.8.1.8? → approval1.8.1.8+
Checked in on the branch.
Keywords: fixed1.8.1.8
The combined patch for bug 267833, bug 373756, bug 394676 and bug 394014 broke the display of the views in the Calendar applications (Lightning and Sunbird). See bug 375390. 

This has been a known issue on the trunk for quite some time, but as we develop mainly on the branch right now (for TB 1.5 and TB2 compatibility reasons) this only occurred on our builds today. This is a serious regression for us, which basically hinders us to release our 0.7 release.

Can this please be backed out and be postponed until we release 0.7 (scheduled for Mid-October)?

CC'ing some key Calendar developers.
After this checkin the issue reported in Bug 375390 happens now in MOZILLA_1_8_BRANCH builds too.
I don't know if the calendar code triggers a bug in the xbl/css code or if the previous working behavior unintentional depended on a bug that is now fixed.

(In reply to comment #67)
> Can this please be backed out and be postponed until we 
> release 0.7 (scheduled for Mid-October)?

This makes no sense unless it's backed out from branch forever.
This sounds like it's going to bust some extensions, too.  Do we need to bump the third digit here?  I'm pretty nervous, if it broke Calendar.
(In reply to comment #68)
>> Can this please be backed out and be postponed until we release 0.7 (scheduled 
>> for Mid-October)?
> 
> This makes no sense unless it's backed out from branch forever.

IMO it makes sense, because then we could at least analyze the issue with sufficient time and develop a fix on our side.

> This has been a known issue on the trunk for quite some time

I don't see bug 375390 in the dep list for this bug, nor was I cced on it.  I can only assume no one took the trouble to hunt down the regression range on trunk?

The whole point of baking things on trunk is to get things tested and fix issues before landing on branch.  But we can't do that if people don't bother to report the issues and then go into panic mode when the change (regression-free as far as anyone knows) hits branch....

Backing this out reopens security holes in Firefox.  I'm frankly not quite sure we should hold that hostage to Calendar, especially since they've been sitting on the bug in question since March...  I would prefer that we actually fix the problem instead, but I'll need some help from the calendar folks here.  Further comments in bug 375390.
(In reply to comment #67)
> See bug 375390. 
> 
> This has been a known issue on the trunk for quite some time,

It would have been nice if that were marked with a regression keyword and linked to one of these bugs, just to give the branch release triagers a fighting chance...

(In reply to comment #69)
> This sounds like it's going to bust some extensions, too.  Do we need to bump
> the third digit here?  I'm pretty nervous, if it broke Calendar.

bumping the 3rd digit busts _all_ extensions which seems much worse than maybe breaking some. Unless you're volunteering the AMO team to test them and bump the maxVersions for the authors?

Can we identify a code pattern of what broke in Calendar and then search AMO for other instances of that pattern? We may indeed back this out of the 1.8.1.8 release in favor of more testing, but even if we do we'll want it back in 1.8.1.9
Depends on: 375390
Note that we've been trying to get this fix into the security releases since April. The delay was for the sole purpose of shaking out and fixing regressions and six months is longer than I wanted to sit on this in the first place.
No longer depends on: 375390
Depends on: 375390
> > This sounds like it's going to bust some extensions, too.  Do we need to bump
> > the third digit here?  I'm pretty nervous, if it broke Calendar.
> 
> bumping the 3rd digit busts _all_ extensions which seems much worse than maybe
> breaking some. Unless you're volunteering the AMO team to test them and bump
> the maxVersions for the authors?

Bumping the 3rd digit is what we've promised to do if we make incompatible changes, and it disables rather than randomly permuting behaviour (risking data loss and other instabilities).  I can't volunteer any AMO team for anything, but if we're planning to take a change on the stability branch that needs compat updates, then I think we have to bump the version and get the word out that we're doing so, with builds extension authors can test against.  What is the specific nature of the breakage?

I presume we didn't know that there were incompatibilities until recently, because those six months of baking would have been a great time to reach out to extension authors. :(
> What is the specific nature of the breakage?

See bug 375390.  It's pretty thin on detail, though.

> I presume we didn't know that there were incompatibilities 

The aim was not to ship any, basically.  We fixed all the ones we found or thought of before the patch landed.
(In reply to comment #75)
> The aim was not to ship any, basically.  We fixed all the ones we found or
> thought of before the patch landed.

That's what I thought, great.  What builds should we be pointing extension authors at to test, and is anyone doing that pointing?
We're sorry fro bringing this up so late, but this only came up on trunk, when we rewrote some of our code for displaying events at the end of March and since we do our development solely on the branch (for the mentioned TB compatibility reasons) this didn't really bother us, since we suspected a bug on our side.
Mike, they should be testing the branch nightlies.  And I'm not sure anyone is pointing right now.
See bug 375390 comment 24.  There _is_ an inadvertent compat change here.  It's arguably for the better, so we should keep it on trunk, but on branch I can try to go back to the old way.  Thoughts?
Depends on: 398404
I filed bug 398404 on the issue I mention in comment 79.
(In reply to comment #79)
> See bug 375390 comment 24.  There _is_ an inadvertent compat change here.  It's
> arguably for the better, so we should keep it on trunk, but on branch I can try
> to go back to the old way.  Thoughts?
> 
So should the trunk change be documented then, given that fix in bug 398404 is branch-only? Or is it still not decided?
> So should the trunk change be documented then

Yes.  I'll add dev-doc-needed to bug 398404, since it's actually open and all.
Depends on: 398422
No longer depends on: 398422
Depends on: 398837
Previously trunk-only Bug 392936 seems to be regressed on 1.8 branch too after the checkin for this bug. More information can be provided in Bug 392936 if required.
Depends on: 392936
No longer blocks: 287981
Flags: blocking1.8.0.14? → blocking1.8.0.14-
Attachment #306269 - Flags: approval1.8.0.15?
Attachment #306269 - Flags: approval1.8.0.15?
Comment on attachment 280936 [details] [diff] [review]
Merged to branch

a=asac for 1.8.0.15
Attachment #280936 - Flags: approval1.8.0.14? → approval1.8.0.15+
Flags: blocking1.8.0.15-
Keywords: checkin-needed
Flags: blocking1.8.0.15- → blocking1.8.0.15+
Checking in content/base/src/nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.566.2.6.2.18; previous revision: 3.566.2.6.2.17
done
Checking in content/base/src/nsDocument.h;
/cvsroot/mozilla/content/base/src/nsDocument.h,v  <--  nsDocument.h
new revision: 3.264.2.3.2.6; previous revision: 3.264.2.3.2.5
done
Checking in content/html/document/src/nsHTMLContentSink.cpp;
/cvsroot/mozilla/content/html/document/src/nsHTMLContentSink.cpp,v  <--  nsHTMLContentSink.cpp
new revision: 3.719.4.4.2.3; previous revision: 3.719.4.4.2.2
done
Checking in content/xbl/src/nsBindingManager.cpp;
/cvsroot/mozilla/content/xbl/src/nsBindingManager.cpp,v  <--  nsBindingManager.cpp
new revision: 1.136.2.1.4.4; previous revision: 1.136.2.1.4.3
done
Checking in content/xbl/src/nsBindingManager.h;
/cvsroot/mozilla/content/xbl/src/nsBindingManager.h,v  <--  nsBindingManager.h
new revision: 1.2.16.2; previous revision: 1.2.16.1
done
Checking in content/xbl/src/nsXBLResourceLoader.cpp;
/cvsroot/mozilla/content/xbl/src/nsXBLResourceLoader.cpp,v  <--  nsXBLResourceLoader.cpp
new revision: 1.34.12.1; previous revision: 1.34
done
Checking in content/xul/templates/src/nsXULContentBuilder.cpp;
/cvsroot/mozilla/content/xul/templates/src/nsXULContentBuilder.cpp,v  <--  nsXULContentBuilder.cpp
new revision: 1.66.4.3.2.2; previous revision: 1.66.4.3.2.1
done
Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1110.6.12.2.66; previous revision: 1.1110.6.12.2.65
done
Checking in layout/base/nsCSSFrameConstructor.h;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.h,v  <--  nsCSSFrameConstructor.h
new revision: 1.187.6.3.2.7; previous revision: 1.187.6.3.2.6
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.852.2.11.2.10; previous revision: 3.852.2.11.2.9
done
That was for MOZILLA_1_8_0_BRANCH...
You need to log in before you can comment on or make changes to this bug.