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

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Core
DOM: Events
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: hsivonen, Assigned: bz)

Tracking

({verified1.9.0.18, verified1.9.1})

Trunk
mozilla1.9.3a1
verified1.9.0.18, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.2 -
blocking1.9.1 -
wanted1.9.1 +
blocking1.9.0.18 +
wanted1.9.0.x +
in-testsuite ?

Firefox Tracking Flags

(status1.9.2 beta4-fixed, blocking1.9.1 .8+, status1.9.1 .8-fixed)

Details

(Whiteboard: [sg:critical?])

Attachments

(5 attachments)

(Reporter)

Description

9 years ago
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

9 years ago
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
(Reporter)

Updated

9 years ago
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?
OK, the patch in bug 444322 does NOT fix this.
And this is really bad, for what it's worth, if iframes get involved.
Flags: blocking1.9.1?
(Reporter)

Updated

9 years ago
Summary: Double items is layout when alert() called from mutation event handler → Double items in layout when alert() called from mutation event handler
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
Assignee: nobody → bzbarsky
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
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).
(Reporter)

Comment 7

9 years ago
HTML5 says that alert() pauses scripts and event handlers but doesn’t say anything about pausing the parser. Perhaps it should.

Updated

8 years ago
Whiteboard: [sg:critical?]
Boris, any chance you can take another look at this?  Does Henri's comment 7 help you move forward?
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.
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?
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.
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.
Blocks: 520820
Filed bug 520820 on the HTML5 issue.
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?  ;)
Attachment #404858 - Flags: review?(jonas)
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.
Attachment #404858 - Flags: review?(jonas) → review+
> 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.
And once that happens, we should back this patch out....
Ooh, very true indeed!
Pushed http://hg.mozilla.org/mozilla-central/rev/d68aad724ff6

We probably need this fix on 1.9.2, as well as 1.9.1....
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite?
Flags: blocking1.9.2?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
blocking1.9.1: --- → ?
blocking1.9.1: ? → .7+
status1.9.1: --- → wanted
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.17+
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.
Flags: blocking1.9.2? → blocking1.9.2-
Attachment #404858 - Flags: approval1.9.2+
Created attachment 413398 [details] [diff] [review]
1.9.2 branch merge
Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/de79f09e0ee5
status1.9.2: --- → final-fixed
Created attachment 413508 [details] [diff] [review]
1.9.1 branch merge
Attachment #413508 - Flags: approval1.9.1.7?
Created attachment 413522 [details] [diff] [review]
1.9.0 branch merge
Attachment #413522 - Flags: approval1.9.0.17?
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.
Attachment #413522 - Flags: superreview?(jst)
Attachment #413508 - Flags: superreview?(jst)
I could call it 1_9_1 across all three branches.  I'd definitely like to have the same name for all three.

Updated

8 years ago
Attachment #413508 - Flags: superreview?(jst) → superreview+
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...
Attachment #413522 - Flags: superreview?(jst) → superreview+
I think answering that question is why dveditz asked you for review.  See comment 26.
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.
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.
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.  ;)
Attachment #413522 - Flags: approval1.9.0.17? → approval1.9.0.17+
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
Attachment #413508 - Flags: approval1.9.1.7? → approval1.9.1.7+
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a245d427e351
status1.9.1: wanted → .7-fixed
Checked in on 1.9.0 branch.
Keywords: fixed1.9.0.17

Comment 35

8 years ago
Hm, the code is pretty different from what we have in 1.8...any hint how to back-port it?
I don't know how to do this on 1.8 branch offhand.... Do we even have script blockers there?

Comment 37

8 years ago
(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.
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)...
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
Keywords: verified1.9.1
OS: Mac OS X → All
Hardware: x86 → All

Comment 40

8 years ago
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.
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.
Keywords: fixed1.9.0.18 → verified1.9.0.18
Group: core-security
You need to log in before you can comment on or make changes to this bug.