467005 - Double items in layout when alert() called from mutation event handler

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Henri Sivonen (:hsivonen)]

Attachment: 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][[e-mail]]]

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 [:bzbarsky]]

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 [:bzbarsky]]

OK, the patch in bug 444322 does NOT fix this.

Comment 4

[Boris Zbarsky [:bzbarsky]]

And this is really bad, for what it's worth, if iframes get involved.

Comment 5

[Johnny Stenback (:jst)]

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 [:bzbarsky]]

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)]

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)]

Boris, any chance you can take another look at this?  Does Henri's comment 7 help you move forward?

Comment 9

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Filed bug 520820 on the HTML5 issue.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: 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) No longer reading bugmail consistently]

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 [:bzbarsky]]

> 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 [:bzbarsky]]

And once that happens, we should back this patch out....

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ooh, very true indeed!

Comment 19

[Boris Zbarsky [:bzbarsky]]

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]]

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 [:bzbarsky]]

Attachment: 1.9.2 branch merge

Comment 22

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/de79f09e0ee5

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.1 branch merge

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.9.0 branch merge

Comment 25

[Daniel Veditz [:dveditz]]

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 [:bzbarsky]]

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)]

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 [:bzbarsky]]

I think answering that question is why dveditz asked you for review.  See comment 26.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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) No longer reading bugmail consistently]

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 [:bzbarsky]]

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]]

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 [:bzbarsky]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a245d427e351

Comment 34

[Boris Zbarsky [:bzbarsky]]

Checked in on 1.9.0 branch.

Comment 35

[Martin Stránský [:stransky] (ni? me)]

Hm, the code is pretty different from what we have in 1.8...any hint how to back-port it?

Comment 36

[Boris Zbarsky [:bzbarsky]]

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ý [:stransky] (ni? me)]

(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 [:bzbarsky]]

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][⌚️UTC+1]]

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ý [:stransky] (ni? me)]

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 - ex-MoCo]]

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.