Last Comment Bug 267833 - [FIX]Fire XBL constructors from EndUpdate(), not before
: [FIX]Fire XBL constructors from EndUpdate(), not before
Status: RESOLVED FIXED
: fixed1.8.0.15, fixed1.8.1.8
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: x86 Linux
: P1 normal with 1 vote (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz]
: Hixie (not reading bugmail)
Mentors:
Depends on: 372729 373719 373721 373727 373756 375390 392936 394676 398404 398837 400735 401743
Blocks: 194952 228557 281657 285732 330445 330563 332807 336960 344434 354645 360599 363280 373944 374022 374166 387844
  Show dependency treegraph
 
Reported: 2004-11-04 17:17 PST by Boris Zbarsky [:bz]
Modified: 2008-03-08 05:38 PST (History)
36 users (show)
jonas: blocking1.9+
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.14-
asac: blocking1.8.0.next+
dveditz: wanted1.8.0.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Something like this (9.06 KB, patch)
2005-01-23 21:49 PST, Boris Zbarsky [:bz]
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Splinter Review
Proposed fix (23.37 KB, patch)
2007-02-02 19:09 PST, Boris Zbarsky [:bz]
enndeakin: review+
Details | Diff | Splinter Review
With that change (29.41 KB, patch)
2007-03-05 11:50 PST, Boris Zbarsky [:bz]
jonas: review+
jonas: superreview+
Details | Diff | Splinter Review
Updated to comments (29.30 KB, patch)
2007-03-06 11:07 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Merged to branch (27.41 KB, patch)
2007-09-14 14:23 PDT, Boris Zbarsky [:bz]
jonas: review+
jonas: superreview+
dveditz: approval1.8.1.8+
asac: approval1.8.0.next+
Details | Diff | Splinter Review
same diffed against 1.8.0 (25.51 KB, patch)
2008-02-28 06:40 PST, Alexander Sack
no flags Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2004-11-04 17:17:22 PST
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?
Comment 1 Boris Zbarsky [:bz] 2005-01-23 21:49:50 PST
Created attachment 172234 [details] [diff] [review]
Something like this
Comment 2 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-01-24 08:43:21 PST
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.
Comment 3 Boris Zbarsky [:bz] 2005-01-24 09:19:14 PST
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 Brian Ryner (not reading) 2005-01-27 09:05:44 PST
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 5 Boris Zbarsky [:bz] 2005-01-27 09:13:46 PST
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.  :(
Comment 6 Boris Zbarsky [:bz] 2006-11-02 22:13:37 PST
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...
Comment 7 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-11-03 00:48:36 PST
No, the sink still doesn't call Begin/EndUpdate, I just fixed nsGenericDOMDataNode.
Comment 8 Boris Zbarsky [:bz] 2006-11-03 06:43:46 PST
Hmm...  I thought I'd reviewed a patch where you added begin/end update calls to or around FlushTags...
Comment 9 Mike Schroepfer 2007-01-30 14:27:42 PST
Hey Bz/Sicking - are we still planning on this for 1.9?
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-01-30 14:53:38 PST
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.
Comment 11 Boris Zbarsky [:bz] 2007-02-02 17:50:13 PST
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.
Comment 12 Boris Zbarsky [:bz] 2007-02-02 17:55:57 PST
And can I say.... _wow_ that patch is against old code.  ;)
Comment 13 Boris Zbarsky [:bz] 2007-02-02 19:09:13 PST
Created attachment 253827 [details] [diff] [review]
Proposed fix

This is a lot more careful than the first patch...

Neil, any suggestions for testing the template builder stuff?
Comment 14 Neil Deakin 2007-02-07 09:16:19 PST
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.
Comment 15 Boris Zbarsky [:bz] 2007-02-07 11:30:25 PST
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 Neil Deakin 2007-02-07 12:00:38 PST
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.
Comment 17 Boris Zbarsky [:bz] 2007-02-07 15:11:11 PST
Yeah, I figure sicking will review that.  :)
Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-03-01 18:05:01 PST
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.
Comment 19 Boris Zbarsky [:bz] 2007-03-01 18:43:38 PST
> 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.
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-03-02 19:01:24 PST
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 :(
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-03-02 19:09:29 PST
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.
Comment 22 Boris Zbarsky [:bz] 2007-03-02 21:18:45 PST
> 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.  ;)
Comment 23 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-03-03 01:12:15 PST
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.
Comment 24 Boris Zbarsky [:bz] 2007-03-04 12:57:38 PST
> 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?
Comment 25 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-03-04 17:30:59 PST
If you've got the cycles it'd be great if you could give it a try.
Comment 26 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-03-04 17:35:20 PST
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.
Comment 27 Boris Zbarsky [:bz] 2007-03-04 17:58:11 PST
Yeah, indeed.  That would be the plan.

I'll try to give this a show sometime this week.
Comment 28 Boris Zbarsky [:bz] 2007-03-05 11:50:59 PST
Created attachment 257403 [details] [diff] [review]
With that change

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.
Comment 29 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-03-05 15:54:41 PST
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.
Comment 30 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-03-05 15:57:49 PST
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
Comment 31 Boris Zbarsky [:bz] 2007-03-06 11:04:22 PST
> 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.
Comment 32 Boris Zbarsky [:bz] 2007-03-06 11:07:49 PST
Created attachment 257535 [details] [diff] [review]
Updated to comments
Comment 33 Boris Zbarsky [:bz] 2007-03-06 14:36:11 PST
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....
Comment 34 chris hofmann 2007-03-08 11:18:11 PST
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?
Comment 35 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-03-08 14:32:30 PST
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.
Comment 36 Boris Zbarsky [:bz] 2007-03-08 21:13:51 PST
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...
Comment 37 Boris Zbarsky [:bz] 2007-03-09 21:05:33 PST
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.  :(
Comment 38 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-03-10 00:04:30 PST
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.
Comment 39 Boris Zbarsky [:bz] 2007-03-12 09:10:36 PDT
> 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 Dietrich Ayala (:dietrich) 2007-03-13 22:04:39 PDT
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 (not reading, please use seth@sspitzer.org instead) 2007-03-14 17:53:36 PDT
for the places regression (you need --enable-places-bookmarks to see it) see bug #373944 for the workaround.
Comment 42 Daniel Veditz [:dveditz] 2007-06-13 11:42:29 PDT
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)
Comment 43 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-06-13 11:57:44 PDT
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?
Comment 44 Boris Zbarsky [:bz] 2007-06-13 14:08:32 PDT
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 (not reading, please use seth@sspitzer.org instead) 2007-06-19 17:47:37 PDT
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.
Comment 46 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-06-19 17:50:54 PDT
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 (not reading, please use seth@sspitzer.org instead) 2007-06-20 00:27:33 PDT
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 (not reading, please use seth@sspitzer.org instead) 2007-06-20 00:59:01 PDT
> 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 (not reading, please use seth@sspitzer.org instead) 2007-06-20 15:49:25 PDT
> 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
Comment 50 Daniel Veditz [:dveditz] 2007-07-09 15:40:53 PDT
Too close to 2.0.0.5 code-freeze to take given the number of regressions.
Comment 51 Boris Zbarsky [:bz] 2007-07-09 18:38:12 PDT
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.
Comment 52 Daniel Veditz [:dveditz] 2007-09-06 16:38:11 PDT
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.
Comment 53 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-09-06 17:09:53 PDT
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.
Comment 54 Boris Zbarsky [:bz] 2007-09-14 14:23:37 PDT
Created attachment 280936 [details] [diff] [review]
Merged to branch

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.
Comment 55 georgi - hopefully not receiving bugspam 2007-09-15 01:49:11 PDT
bz, do you expect some xbl functionality to cause use after free after this patch?
Comment 56 Boris Zbarsky [:bz] 2007-09-15 10:38:48 PDT
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 georgi - hopefully not receiving bugspam 2007-09-16 01:39:54 PDT
(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. 
Comment 58 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-09-20 21:04:26 PDT
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+
Comment 59 Boris Zbarsky [:bz] 2007-09-20 21:11:00 PDT
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.
Comment 60 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-09-20 21:16:52 PDT
Hmm.. what's the downside of adding it? I'm mostly scared about doing something different on branch than trunk here.
Comment 61 Boris Zbarsky [:bz] 2007-09-20 21:23:02 PDT
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 62 Jonas Sicking (:sicking) No longer reading bugmail consistently 2007-09-20 21:24:53 PDT
Comment on attachment 280936 [details] [diff] [review]
Merged to branch

cool! r/sr=me
Comment 63 Daniel Veditz [:dveditz] 2007-09-24 12:12:10 PDT
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?

Comment 64 Boris Zbarsky [:bz] 2007-09-24 12:17:39 PDT
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 Daniel Veditz [:dveditz] 2007-09-26 10:22:22 PDT
Comment on attachment 280936 [details] [diff] [review]
Merged to branch

approved for 1.8.1.8, a=dveditz for release-drivers
Comment 66 Boris Zbarsky [:bz] 2007-09-30 19:46:26 PDT
Checked in on the branch.
Comment 67 Simon Paquet [:sipaq] 2007-10-02 09:36:54 PDT
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 Stefan Sitter 2007-10-02 09:45:05 PDT
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.
Comment 69 Mike Shaver (:shaver -- probably not reading bugmail closely) 2007-10-02 09:51:22 PDT
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 Simon Paquet [:sipaq] 2007-10-02 09:53:35 PDT
(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.

Comment 71 Boris Zbarsky [:bz] 2007-10-02 10:06:00 PDT
> 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 Daniel Veditz [:dveditz] 2007-10-02 10:08:47 PDT
(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 Daniel Veditz [:dveditz] 2007-10-02 10:13:20 PDT
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.
Comment 74 Mike Shaver (:shaver -- probably not reading bugmail closely) 2007-10-02 10:48:34 PDT
> > 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. :(
Comment 75 Boris Zbarsky [:bz] 2007-10-02 10:56:01 PDT
> 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.
Comment 76 Mike Shaver (:shaver -- probably not reading bugmail closely) 2007-10-02 11:04:43 PDT
(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 Simon Paquet [:sipaq] 2007-10-02 11:18:58 PDT
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.
Comment 78 Boris Zbarsky [:bz] 2007-10-02 12:39:20 PDT
Mike, they should be testing the branch nightlies.  And I'm not sure anyone is pointing right now.
Comment 79 Boris Zbarsky [:bz] 2007-10-02 14:37:30 PDT
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?
Comment 80 Boris Zbarsky [:bz] 2007-10-03 07:38:29 PDT
I filed bug 398404 on the issue I mention in comment 79.
Comment 81 Nickolay_Ponomarev 2007-10-04 01:12:58 PDT
(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?
Comment 82 Boris Zbarsky [:bz] 2007-10-04 08:20:58 PDT
> 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 Stefan Sitter 2007-10-30 01:18:54 PDT
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.
Comment 84 Alexander Sack 2008-02-28 06:40:20 PST
Created attachment 306269 [details] [diff] [review]
same diffed against 1.8.0
Comment 85 Alexander Sack 2008-02-28 06:41:03 PST
Comment on attachment 280936 [details] [diff] [review]
Merged to branch

a=asac for 1.8.0.15
Comment 86 Reed Loden [:reed] (use needinfo?) 2008-03-08 05:37:55 PST
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
Comment 87 Reed Loden [:reed] (use needinfo?) 2008-03-08 05:38:15 PST
That was for MOZILLA_1_8_0_BRANCH...

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