Closed Bug 537683 Opened 10 years ago Closed 10 years ago

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

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: hsivonen)

References

()

Details

Attachments

(1 file, 2 obsolete files)

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.
Priority: -- → P1
My guess above about the cause was incorrect, because this patch doesn't fix the problem.
Is there a bug on the same testcase sending the number of removable scriptblockers negative (hence overflowing it, and locking up the browser)?
(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).
Summary: [HTML5] docshell/base/crashtests/432114-1.html unblock onload too many times → [HTML5] docshell/base/crashtests/432114-1.html removes script blockers too many times
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.
Assignee: nobody → hsivonen
Attachment #419892 - Attachment is obsolete: true
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.
Attachment #420511 - Attachment is obsolete: true
Attachment #420727 - Flags: review?(bnewman)
Summary: [HTML5] docshell/base/crashtests/432114-1.html removes script blockers too many times → [HTML5][Patch] docshell/base/crashtests/432114-1.html removes script blockers too many times
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.
Attachment #420727 - Flags: review?(bnewman) → review+
http://hg.mozilla.org/mozilla-central/rev/8f300ec399cc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.