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

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

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

Updated

9 years ago
Priority: -- → P1
(Assignee)

Comment 1

9 years ago
Created attachment 419892 [details] [diff] [review]
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.
Is there a bug on the same testcase sending the number of removable scriptblockers negative (hence overflowing it, and locking up the browser)?
Blocks: 373864
(Assignee)

Comment 3

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

Comment 4

9 years ago
Created attachment 420511 [details] [diff] [review]
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.
Assignee: nobody → hsivonen
Attachment #419892 - Attachment is obsolete: true
(Assignee)

Comment 5

9 years ago
Created attachment 420727 [details] [diff] [review]
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.
Attachment #420511 - Attachment is obsolete: true
Attachment #420727 - Flags: review?(bnewman)
(Assignee)

Updated

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

Comment 7

9 years ago
http://hg.mozilla.org/mozilla-central/rev/8f300ec399cc
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.