537683 - [HTML5][Patch] docshell/base/crashtests/432114-1.html removes script blockers too many times

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Henri Sivonen (:hsivonen)]

Steps to reproduce (in a debug build):
 1) Enable HTML5 parser.
 2) Load http://mxr.mozilla.org/mozilla-central/source/docshell/base/crashtests/432114-1.html

Actual results:
Assertion about too many attempts to unblock onload fires.

Expected results:
No assertion failure.

Probable cause:
http://hg.mozilla.org/mozilla-central/file/01f4b785c886/parser/html/nsHtml5TreeOpExecutor.h#l293 might assume too much about the stability of the mFlushState value during notifications.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Patch for addressing the probable cause mentioned in the bug report; does not fix the bug

My guess above about the cause was incorrect, because this patch doesn't fix the problem.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Is there a bug on the same testcase sending the number of removable scriptblockers negative (hence overflowing it, and locking up the browser)?

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to comment #2)
> Is there a bug on the same testcase sending the number of removable
> scriptblockers negative (hence overflowing it, and locking up the browser)?

This is supposed to be that bug. I observed the same as you did when debugging, but wrote about the wrong kind of blocker when filing the bug (maybe I was thinking about previous parse end bugs too much to confuse the blockers when writing).

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Potential fix: Make DidBuildModel not break out of update batch

So, the initial bug was that EndDocUpdate could cause a call to Terminate, which in turn re-entered EndDocUpdate before the earlier EndDocUpdate on the stack had adjusted mFlushState. However, making EndDocUpdate adjust mFlushState earlier caused the other batch end win so that the termination destroyed the frame constructor from within the update batch.

Since the old parser must be getting Terminate calls while an update batch is open, I figured that it's not necessary to make DidBuildModel break out of the update batch. As a result, the need to handle an EndDocUpdate while firing append notifications goes away.

Not requesting review yet, because I want to test this some more first.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Break out of update on EOF but not on Terminate

Notes:
 * The added opqueue clear is needed when Terminate() is called before document.write() returns (or alternatively, the destructor could be made not assert on op queue, but it seems useful to have that diagnostic there).

 * Normal EOF needs to break out of the update batch in order to be able to run scripts. However, Terminate() doesn't run scripts anyway, so letting terminations go through inside an update batch seems OK.

Comment 6

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 420727 [details] [diff] [review]
Break out of update on EOF but not on Terminate

Indeed, re-entry of EndDocUpdate no longer occurs.  Looks good to me.

Comment 7

[Henri Sivonen (:hsivonen)]

http://hg.mozilla.org/mozilla-central/rev/8f300ec399cc