Closed
Bug 267833
Opened 20 years ago
Closed 18 years ago
[FIX]Fire XBL constructors from EndUpdate(), not before
Categories
(Core :: XBL, defect, P1)
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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
sicking
:
review+
sicking
:
superreview+
dveditz
:
approval1.8.1.8+
asac
:
approval1.8.0.next+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•20 years ago
|
||
Attachment #172234 -
Flags: superreview?(shaver)
Attachment #172234 -
Flags: review?(bryner)
Assignee | ||
Updated•20 years ago
|
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.
Assignee | ||
Comment 3•20 years ago
|
||
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 4•20 years ago
|
||
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.
Assignee | ||
Comment 5•20 years ago
|
||
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-
Assignee | ||
Updated•20 years ago
|
Summary: [FIX]Fire XBL constructors from EndUpdate(), not before → Fire XBL constructors from EndUpdate(), not before
Target Milestone: mozilla1.8beta → mozilla1.9alpha
Assignee | ||
Updated•19 years ago
|
Assignee | ||
Comment 6•18 years ago
|
||
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...
Assignee | ||
Updated•18 years ago
|
Flags: blocking1.9?
No, the sink still doesn't call Begin/EndUpdate, I just fixed nsGenericDOMDataNode.
Assignee | ||
Comment 8•18 years ago
|
||
Hmm... I thought I'd reviewed a patch where you added begin/end update calls to or around FlushTags...
Comment 9•18 years ago
|
||
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.
Assignee | ||
Comment 11•18 years ago
|
||
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.
Assignee | ||
Comment 12•18 years ago
|
||
And can I say.... _wow_ that patch is against old code. ;)
Assignee | ||
Comment 13•18 years ago
|
||
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)
Assignee | ||
Updated•18 years ago
|
Summary: Fire XBL constructors from EndUpdate(), not before → [FIX]Fire XBL constructors from EndUpdate(), not before
Comment 14•18 years ago
|
||
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.
Assignee | ||
Comment 15•18 years ago
|
||
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 16•18 years ago
|
||
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+
Assignee | ||
Comment 17•18 years ago
|
||
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.
Assignee | ||
Comment 19•18 years ago
|
||
> 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.
Assignee | ||
Comment 22•18 years ago
|
||
> 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.
Assignee | ||
Comment 24•18 years ago
|
||
> 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.
Assignee | ||
Comment 27•18 years ago
|
||
Yeah, indeed. That would be the plan.
I'll try to give this a show sometime this week.
Assignee | ||
Comment 28•18 years ago
|
||
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
Assignee | ||
Comment 31•18 years ago
|
||
> 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.
Assignee | ||
Comment 32•18 years ago
|
||
Attachment #257403 -
Attachment is obsolete: true
Assignee | ||
Comment 33•18 years ago
|
||
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
Comment 34•18 years ago
|
||
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.
Assignee | ||
Comment 36•18 years ago
|
||
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...
Assignee | ||
Comment 37•18 years ago
|
||
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: 18 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.
Assignee | ||
Comment 39•18 years ago
|
||
> 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...
Comment 40•18 years ago
|
||
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.
Comment 41•18 years ago
|
||
for the places regression (you need --enable-places-bookmarks to see it) see bug #373944 for the workaround.
Blocks: 374022
Updated•18 years ago
|
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
Comment 42•18 years ago
|
||
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?
Assignee | ||
Comment 44•18 years ago
|
||
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.
Comment 45•18 years ago
|
||
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.
Comment 47•18 years ago
|
||
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?
Comment 48•18 years ago
|
||
> 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.
Comment 49•18 years ago
|
||
> 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
Updated•18 years ago
|
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
Comment 50•18 years ago
|
||
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+
Assignee | ||
Comment 51•18 years ago
|
||
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.
Assignee | ||
Updated•18 years ago
|
Attachment #257535 -
Flags: approval1.8.1.6?
Updated•17 years ago
|
Flags: blocking1.8.0.13+ → blocking1.8.0.14?
Comment 52•17 years ago
|
||
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.
Assignee | ||
Comment 54•17 years ago
|
||
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?
Comment 55•17 years ago
|
||
bz, do you expect some xbl functionality to cause use after free after this patch?
Assignee | ||
Comment 56•17 years ago
|
||
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?
Comment 57•17 years ago
|
||
(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+
Assignee | ||
Comment 59•17 years ago
|
||
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.
Assignee | ||
Comment 61•17 years ago
|
||
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+
Updated•17 years ago
|
Attachment #257535 -
Flags: approval1.8.1.8+
Comment 63•17 years ago
|
||
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?
Assignee | ||
Comment 64•17 years ago
|
||
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 65•17 years ago
|
||
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+
Comment 67•17 years ago
|
||
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.
Comment 68•17 years ago
|
||
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.
Comment 70•17 years ago
|
||
(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.
Assignee | ||
Comment 71•17 years ago
|
||
> 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.
Comment 72•17 years ago
|
||
(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
Comment 73•17 years ago
|
||
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
> > 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. :(
Assignee | ||
Comment 75•17 years ago
|
||
> 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?
Comment 77•17 years ago
|
||
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.
Assignee | ||
Comment 78•17 years ago
|
||
Mike, they should be testing the branch nightlies. And I'm not sure anyone is pointing right now.
Assignee | ||
Comment 79•17 years ago
|
||
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?
Assignee | ||
Comment 80•17 years ago
|
||
I filed bug 398404 on the issue I mention in comment 79.
Comment 81•17 years ago
|
||
(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?
Assignee | ||
Comment 82•17 years ago
|
||
> 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.
Comment 83•17 years ago
|
||
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
Depends on: 401743
Updated•17 years ago
|
Flags: blocking1.8.0.14? → blocking1.8.0.14-
Comment 84•17 years ago
|
||
Attachment #306269 -
Flags: approval1.8.0.15?
Updated•17 years ago
|
Attachment #306269 -
Flags: approval1.8.0.15?
Comment 85•17 years ago
|
||
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+
Updated•17 years ago
|
Flags: blocking1.8.0.15-
Keywords: checkin-needed
Updated•17 years ago
|
Flags: blocking1.8.0.15- → blocking1.8.0.15+
Comment 86•17 years ago
|
||
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
Keywords: checkin-needed → fixed1.8.0.15
Comment 87•17 years ago
|
||
That was for MOZILLA_1_8_0_BRANCH...
You need to log in
before you can comment on or make changes to this bug.
Description
•