Last Comment Bug 467005 - Double items in layout when alert() called from mutation event handler
: Double items in layout when alert() called from mutation event handler
Status: RESOLVED FIXED
[sg:critical?]
: verified1.9.0.18, verified1.9.1
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.3a1
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 520820
  Show dependency treegraph
 
Reported: 2008-11-27 09:28 PST by Henri Sivonen (:hsivonen)
Modified: 2010-06-20 10:04 PDT (History)
15 users (show)
mbeltzner: blocking1.9.2-
jst: blocking1.9.1-
jst: wanted1.9.1+
dveditz: blocking1.9.0.18+
dveditz: wanted1.9.0.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta4-fixed
.8+
.8-fixed


Attachments
Test case that calls alert() from mutation event handler (553 bytes, text/html)
2008-11-27 09:28 PST, Henri Sivonen (:hsivonen)
no flags Details
Proposed patch (14.22 KB, patch)
2009-10-06 10:53 PDT, Boris Zbarsky [:bz]
jonas: review+
mbeltzner: approval1.9.2+
Details | Diff | Review
1.9.2 branch merge (15.82 KB, patch)
2009-11-19 11:33 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Review
1.9.1 branch merge (15.37 KB, patch)
2009-11-19 19:41 PST, Boris Zbarsky [:bz]
jst: superreview+
dveditz: approval1.9.1.8+
Details | Diff | Review
1.9.0 branch merge (15.64 KB, patch)
2009-11-19 21:23 PST, Boris Zbarsky [:bz]
jst: superreview+
dveditz: approval1.9.0.18+
Details | Diff | Review

Description Henri Sivonen (:hsivonen) 2008-11-27 09:28:10 PST
Created attachment 350348 [details]
Test case that calls alert() from mutation event handler

Build id: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081127 Minefield/3.1b3pre

Steps to reproduce:
Load the attachment.

Actual results:
Content that is inserted once to the DOM shows up twice in layout. (Can't be selected, doesn't show up it Firebug inspector.)

Expected:
Expected things to show up in layout once.
Comment 1 Olli Pettay [:smaug] 2008-11-27 09:30:09 PST
I get some assertions when loading the testcase
###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /mozilla/gfx/thebes/src/gfxSkipChars.cpp, line 92
###!!! ASSERTION: Substring out of range: 'aStart + aLength <= mCharacterCount', file /mozilla/gfx/thebes/src/gfxFont.cpp, line 1775
Comment 2 Boris Zbarsky [:bz] 2008-12-04 17:29:42 PST
Oh, this is great fun.

The key here is that we're starting an update, then inserting the node, then doing the document.write (which reenables the parser) then spinning the event loop for a bit, which causes the parser to parse the text we just wrote, as far as I can tell, and call back into the sink.

At this point the sink goes to notify, but it hasn't updated its child counts for the mutation yet (because the update hasn't ended), so it notifies on the content we've already notified on.

Jonas, could we update the sink's counts when we remove the script blockers preparatory to firing the mutation event?  That would make the most sense to me...

Alternately, should we even be running the parser here before the script finishes executing?  That seems a lot like the issue mrbkap has a fix for with sync XHR and OnDataAvailable, so maybe his patch will fix this?
Comment 3 Boris Zbarsky [:bz] 2008-12-04 18:45:56 PST
OK, the patch in bug 444322 does NOT fix this.
Comment 4 Boris Zbarsky [:bz] 2008-12-05 11:31:14 PST
And this is really bad, for what it's worth, if iframes get involved.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2009-01-15 17:13:09 PST
Yeah, that's exactly what I was thinking, I don't really know a way to do that off the top of my head though.

Wanna have a go?

Note really sure that this is a blocker given that it's not a regression. But it's really bad so we should fix it asap.

/ Jonas
Comment 6 Boris Zbarsky [:bz] 2009-01-15 18:00:56 PST
OK.  I'll give it a shot, but ccing Henri because I'm not sure whether HTML5 covers this situation particularly well (and in particular whether it covers whether the parser should run on the written text while the alert is up, and whether it matters).
Comment 7 Henri Sivonen (:hsivonen) 2009-01-16 00:14:58 PST
HTML5 says that alert() pauses scripts and event handlers but doesn’t say anything about pausing the parser. Perhaps it should.
Comment 8 Brandon Sterne (:bsterne) 2009-10-05 16:33:13 PDT
Boris, any chance you can take another look at this?  Does Henri's comment 7 help you move forward?
Comment 9 Boris Zbarsky [:bz] 2009-10-05 17:40:19 PDT
Yikes.  This was on my todo-list, but got buried under a mountain of other stuff.

So I looked into updating counts off a script runner.  I _think_ we can make it work, as long as we're pretty careful.  I'll try to come up with a patch.  I wish we could just nix all this business with content flushes....

That said, Henri, this testcase triggers asserts in the HTML5 parser.  I assume you want a separate bug on that.
Comment 10 Boris Zbarsky [:bz] 2009-10-05 18:41:28 PDT
So a problem:  Say I have nested updates A and B.  So we get BeginUpdate for A, then BeginUpdate for B.  The former would flush the sink; the latter would not.  Now we pop off both scriptblockers; the sink needs to update its child counts.  Some script executes, possibly triggering nested updates and pumping data into the sink.  The sink needs to FlushTags at the beginnings of those updates, but it's really not set up to do that...

In some ways it would be ideal if the sink flushed when a content update starts, then until the end of that content update updated child counts every time the script blocker count goes to 0 (before anything else happens!) and flushed every time the script blocker count went nonzero.  Jonas, would you be ok with me adding some sort of "unscriptrunner" back-door to nsContentUtils?
Comment 11 Boris Zbarsky [:bz] 2009-10-05 18:53:00 PDT
The other option is to have mozAutoRemovableBlockerRemover take an nsIDocument as an argument and call EndUpdate() on the sink for every blocker it removes, then BeginUpdate for every blocker it restores...  The only caveat is that removable script blockers exactly correspond to UPDATE_CONTENT_MODEL updates, while the content sink cares about all updates other than UPDATE_CONTENT_STATE...  I _think_ it might still work, though.
Comment 12 Boris Zbarsky [:bz] 2009-10-05 18:55:48 PDT
And in particular, the only updates that are not either of those are UPDATE_STYLE, and there should be no mutation event firing inside an UPDATE_STYLE update, that I know of.
Comment 13 Boris Zbarsky [:bz] 2009-10-06 10:49:17 PDT
Filed bug 520820 on the HTML5 issue.
Comment 14 Boris Zbarsky [:bz] 2009-10-06 10:53:01 PDT
Created attachment 404858 [details] [diff] [review]
Proposed patch

Perhaps like so?  I used GetOwnerDocument() to make sure we notify the sink, so with this fix it's possible for mInNotification to go negative (if current documents which are null were used for the begin/end updates before), hence the change to nsContentSink.

If I can get this in on 1.9.2 today I don't have to worry about creating a separate interface, right?  ;)
Comment 15 Jonas Sicking (:sicking) 2009-10-29 17:45:41 PDT
Comment on attachment 404858 [details] [diff] [review]
Proposed patch

Wow, this is gross. But probably what we should do for now.

Ideal is of course to get rid of mutation events. But that'll be a while.

However I do think that we can improve our situation materially if we simply fire them from scriptrunners instead.

But that'll have to be from trunk since that affects behavior slightly.
Comment 16 Boris Zbarsky [:bz] 2009-10-29 17:59:53 PDT
> Ideal is of course to get rid of mutation events. But that'll be a while.

We're more likely to switch to the html5 parser, with its saner notification setup, first.
Comment 17 Boris Zbarsky [:bz] 2009-10-29 18:00:23 PDT
And once that happens, we should back this patch out....
Comment 18 Jonas Sicking (:sicking) 2009-10-29 18:01:03 PDT
Ooh, very true indeed!
Comment 19 Boris Zbarsky [:bz] 2009-10-29 18:54:22 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/d68aad724ff6

We probably need this fix on 1.9.2, as well as 1.9.1....
Comment 20 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-12 08:33:08 PST
As much as it pains me, I don't think we'd block 1.9.2 on this (we could get to it in a security and stability release). That said, the patch seems well baked, so a192=beltzner.
Comment 21 Boris Zbarsky [:bz] 2009-11-19 11:33:57 PST
Created attachment 413398 [details] [diff] [review]
1.9.2 branch merge
Comment 22 Boris Zbarsky [:bz] 2009-11-19 11:38:02 PST
Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/de79f09e0ee5
Comment 23 Boris Zbarsky [:bz] 2009-11-19 19:41:16 PST
Created attachment 413508 [details] [diff] [review]
1.9.1 branch merge
Comment 24 Boris Zbarsky [:bz] 2009-11-19 21:23:34 PST
Created attachment 413522 [details] [diff] [review]
1.9.0 branch merge
Comment 25 Daniel Veditz [:dveditz] 2009-11-30 14:22:45 PST
Comment on attachment 413522 [details] [diff] [review]
1.9.0 branch merge

Requesting sr=jst on a process question

>+#define NS_IDOCUMENT_MOZILLA_1_9_2_BRANCH_IID               \

Are "MOZILLA_1_9_2_BRANCH" names ok for 190/191 branches? Gets points for consistency and potentially easier future merges (if we have more security bugs in the same spot?) but is otherwise kinda confusing.
Comment 26 Boris Zbarsky [:bz] 2009-11-30 14:41:36 PST
I could call it 1_9_1 across all three branches.  I'd definitely like to have the same name for all three.
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-30 16:41:48 PST
Comment on attachment 413522 [details] [diff] [review]
1.9.0 branch merge

+class nsIDocument_MOZILLA_1_9_2_BRANCH : public nsISupports {

Should we name this 1_9_0 instead of 1_9_2? Same thing in the 1.9.1 version as well. Doesn't really matter, and I don't know what we usually do in cases like this. Up to you...
Comment 28 Boris Zbarsky [:bz] 2009-11-30 18:28:43 PST
I think answering that question is why dveditz asked you for review.  See comment 26.
Comment 29 Jonas Sicking (:sicking) 2009-11-30 18:47:50 PST
I would say it doesn't matter much either way. But given the amount of time we spend looking at branch code (very little), I don't think consistency within a branch is very valuable. So I'd give it the same name everywhere to ease porting of this and future patches.
Comment 30 Jonas Sicking (:sicking) 2009-11-30 18:49:25 PST
Just make sure that if the interface has the same iid on several branches, that it has the same vtable. So for example don't give it the same iid if both nsIX_1_9_1_BRANCH interfaces inherit the nsIX interface, but the nsIX interface differs between the branches.
Comment 31 Boris Zbarsky [:bz] 2009-11-30 18:57:45 PST
Yes, the iid is exactly the same on 1.9.2/1.9.1/1.9.0 and the interface inherits from nothing other than nsISupports.  I think I'm good on that front.  ;)
Comment 32 Daniel Veditz [:dveditz] 2009-12-02 15:12:24 PST
Comment on attachment 413522 [details] [diff] [review]
1.9.0 branch merge

Approved for 1.9.1.7 and 1.9.0.17, a=dveditz for release-drivers
Comment 33 Boris Zbarsky [:bz] 2009-12-03 11:01:52 PST
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a245d427e351
Comment 34 Boris Zbarsky [:bz] 2009-12-03 11:11:35 PST
Checked in on 1.9.0 branch.
Comment 35 Martin Stránský 2010-01-27 05:48:40 PST
Hm, the code is pretty different from what we have in 1.8...any hint how to back-port it?
Comment 36 Boris Zbarsky [:bz] 2010-01-27 07:48:21 PST
I don't know how to do this on 1.8 branch offhand.... Do we even have script blockers there?
Comment 37 Martin Stránský 2010-01-27 08:16:17 PST
(In reply to comment #36)
> I don't know how to do this on 1.8 branch offhand.... Do we even have script
> blockers there?

No, script blockers are not there.
Comment 38 Boris Zbarsky [:bz] 2010-01-27 08:25:19 PST
I suppose you could try manually updating the sink's child counts at the mutation event dispatch callsites (and possibly adding an interface to do so, etc)...
Comment 39 Henrik Skupin (:whimboo) 2010-01-28 13:02:03 PST
Verified fixed on 1.9.1 with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.8pre) Gecko/20100125 Shiretoko/3.5.8pre ID:20100125030736

The results I see is now: WRITEWRITE Foo
Comment 40 Martin Stránský 2010-01-29 05:04:39 PST
Hm, I'm afraid it's too difficult/risky for the 1.8 backport, we're not going to ship it in next 1.8.0. update. So please leave this bug confidential after .18 release.
Comment 41 Al Billings [:abillings] 2010-02-05 15:35:01 PST
Verified for 1.9.0.18 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.0.18pre) Gecko/2010020216 GranParadiso/3.0.18pre. I see the same results at Henrik in comment 39.

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