ts harness calls document.write from an onload handler causing the document.write to be counted into shutdown time

RESOLVED FIXED

Status

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: hsivonen, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
Created attachment 443585 [details] [diff] [review]
Remove the bad line

Steps to reproduce:
 1) Put a printf in nsHtmlParser::Parse(const nsAString&, void*, const nsACString&, PRBool, nsDTDMode) if html5.enable=true or nsParser::Parse(const nsAString&, void*, const nsACString&, PRBool, nsDTDMode) if html5.enable=false.
 2) Put a printf in nsAppStartup::Quit
 3) Build the app
 4) Run the ts benchmark with the new build.

Actual results:
nsIParser::Parse(const nsAString&, void*, const nsACString&, PRBool, nsDTDMode) is called after nsAppStartup::Quit.

Expected results:
Expected no call to nsIParser::Parse(const nsAString&, void*, const nsACString&, PRBool, nsDTDMode) after nsAppStartup::Quit.

See also the dev-tree-management thread at http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/4b3ed61fdb8287ac I suspect the document.write() after quit has something to do with the ts shutdown regression seen when the HTML5 parser was turned on by default.
Attachment #443585 - Flags: review?(lsblakk)
(Reporter)

Comment 1

9 years ago
s/nsHtmlParser/nsHtml5Parser/

Updated

9 years ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Reporter)

Updated

9 years ago
Blocks: 564125
Comment on attachment 443585 [details] [diff] [review]
Remove the bad line

sorry for the delay. looks good.
Attachment #443585 - Flags: review?(lsblakk) → review+
(Reporter)

Comment 3

9 years ago
Thanks. I think I don't have access to land this where it needs to land.
Keywords: checkin-needed
Comment on attachment 443585 [details] [diff] [review]
Remove the bad line

Found in triage, just need to land this r+'d patch.
Attachment #443585 - Flags: checked-in?

Updated

8 years ago
Assignee: hsivonen → nobody
Component: Release Engineering → Talos
Flags: checked-in?
Product: mozilla.org → Testing
QA Contact: release → talos
Version: other → unspecified
Can the Talos folks test/land this patch?
I don't quite understand how the document.write is touching the shutdown time since the timestamp is taken after the document.write.
(Reporter)

Comment 7

8 years ago
(In reply to comment #6)
> I don't quite understand how the document.write is touching the shutdown time
> since the timestamp is taken after the document.write.

It seems to me the time taken after the document.write is the end time stamp. Not sure if that get counted towards shutdown exactly, but the use of document.write here is very bogus in any case.
I believe that there is an alternate solution offered here:

bug 648749#c8

Which will let us keep the visual output for when doing manual debugging.
bug 648749 has been deployed, and I believe that it fixes this bug as well.

We now use:

    document.body.textContent = 'Startup time = ' + startupTime + ' ms';

and shutdown is now triggered asynchronously. 

Going to mark this fixed, please re-open if I'm missing something tricky.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Removing checkin-needed due to comment 9.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.