Closed Bug 1333333 Opened 7 years ago Closed 7 years ago

Label runnables in parser/html/

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

Details

Attachments

(1 file)

Bug 1333333 - Label runnables in the HTML parser (again). .
59 bytes, text/x-review-board-request
billm
: review+
Details 
Label the runnables for Quantum DOM.
Any progress on this Henri? FlushTimerCallback is pretty high on the list of runnables we want to label.
Flags: needinfo?(hsivonen)
(In reply to Bill McCloskey (:billm) from comment #2)
> Any progress on this Henri? FlushTimerCallback is pretty high on the list of
> runnables we want to label.

No, I haven't had time to check why everything went orange as of comment 1.
Flags: needinfo?(hsivonen)
billm, any idea why this makes media tests orange in the Quantum Render case?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e5775df65eb&selectedJob=100090666

(I've rebased a couple of times over the past days to account for the possibility of a bad base revision, but the base revision doesn't appear to make a difference.)
Flags: needinfo?(wmccloskey)
I think those tests always fail. They don't appear to run on normal check-in.
Flags: needinfo?(wmccloskey)
Attachment #8867674 - Flags: review?(wmccloskey)
Comment on attachment 8867674 [details]
Bug 1333333 - Label runnables in the HTML parser (again). .

https://reviewboard.mozilla.org/r/139258/#review144736

Thanks!

::: parser/html/nsHtml5RefPtr.h:29
(Diff revision 3)
> -/**
> +  /**
> - * Like nsRefPtr except release is proxied to the main thread. Mostly copied
> - * from nsRefPtr.
> + * Like nsRefPtr except release is proxied to the main
> + * thread. Mostly copied from nsRefPtr.
>   */
> -template <class T>
> -class nsHtml5RefPtr
> +  class nsHtml5RefPtr

The formatting of this file is... really weird. Normally I don't ask for unrelated style fixes, but since you're significantly changing the file anyway, could you fix it? Just running clang-format on it would be a huge improvement.

::: parser/html/nsHtml5RefPtr.h:29
(Diff revision 3)
> -/**
> +  /**
> - * Like nsRefPtr except release is proxied to the main thread. Mostly copied
> - * from nsRefPtr.
> + * Like nsRefPtr except release is proxied to the main
> + * thread. Mostly copied from nsRefPtr.
>   */
> -template <class T>
> -class nsHtml5RefPtr
> +  class nsHtml5RefPtr

The name is a little general for what this is now. Might be nice to include something about StreamParser in it.

::: parser/html/nsHtml5StreamParser.h:390
(Diff revision 3)
>  
> +    /**
> +     * Dispatch an event to a Quantum DOM main thread-ish thread.
> +     * (Not the parser thread.)
> +     */
> +    nsresult DispatchToMainish(const char* aName,

I don't really like the "mainish" name. We'll still have one main event target. You could call it DispatchToMainEventTarget if you don't like DispatchToMain.
Attachment #8867674 - Flags: review?(wmccloskey) → review+
Comment on attachment 8867674 [details]
Bug 1333333 - Label runnables in the HTML parser (again). .

https://reviewboard.mozilla.org/r/139258/#review144736

> The formatting of this file is... really weird. Normally I don't ask for unrelated style fixes, but since you're significantly changing the file anyway, could you fix it? Just running clang-format on it would be a huge improvement.

ran ./mach clang-format.

> The name is a little general for what this is now. Might be nice to include something about StreamParser in it.

Renamed to nsHtml5StreamParserPtr.

> I don't really like the "mainish" name. We'll still have one main event target. You could call it DispatchToMainEventTarget if you don't like DispatchToMain.

Renamed to DispatchToMain.
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d64bdd31c15
Label runnables in the HTML parser. r=billm
https://hg.mozilla.org/mozilla-central/rev/3d64bdd31c15
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1367067
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: Backed out on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8b783001368ba8e915884d7eb5f45ab7cb762f
Merge of the backout:
https://hg.mozilla.org/mozilla-central/rev/fe8b78300136
status-firefox55: fixed → ---
Whiteboard: Backed out on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/fe8b783001368ba8e915884d7eb5f45ab7cb762f
Target Milestone: mozilla55 → ---
Depends on: 1367516
The problem here is that since forever at the end of the parse the circularity between the parser, the sink and the document is broken by the sink forgetting about the document. This is a problem, because the document is needed for targeting runnables. A new fix here should make the document drop its reference to the parser at the end of the parse as the circularity breaker and just raise a boolean flag on the sink to signal the situations that were previously signaled by nulling out the sink's mDocument.
(In reply to Henri Sivonen (:hsivonen) from comment #18)
> The problem here is that since forever at the end of the parse the
> circularity between the parser, the sink and the document is broken by the
> sink forgetting about the document. This is a problem, because the document
> is needed for targeting runnables. A new fix here should make the document
> drop its reference to the parser at the end of the parse as the circularity
> breaker and just raise a boolean flag on the sink to signal the situations
> that were previously signaled by nulling out the sink's mDocument.

The above comment is bogus. The circularity is already broken by the sink dropping the reference to the parser. The sink drops the reference to the document only in cycle collection AFAICT. Also, it seems that nsHtml5StreamParser also nulls out the pointer to the sink (mExecutor) only in cycle collection.
It seems to me that the problem here is cycle collection related:

The crashes occur if the cycle collector has caused either mExecutor of nsHtml5StreamParser or mDocument of nsContentSink to be nulled out.

The fix to that would be making nsHtml5StreamParserPtr take a reference to the DocGroup of the document earlier.
(In reply to Henri Sivonen (:hsivonen) from comment #20)
> It seems to me that the problem here is cycle collection related:
> 
> The crashes occur if the cycle collector has caused either mExecutor of
> nsHtml5StreamParser or mDocument of nsContentSink to be nulled out.
> 
> The fix to that would be making nsHtml5StreamParserPtr take a reference to
> the DocGroup of the document earlier.

Or, I guess, nsHtml5StreamParser itself holding a DocGroup.
(In reply to Henri Sivonen (:hsivonen) from comment #21)
> Or, I guess, nsHtml5StreamParser itself holding a DocGroup.

And everything is orange, because the DocGroup is nullptr at the time of parser initialization.
The Wd failures at https://treeherder.mozilla.org/#/jobs?repo=try&revision=070620887f06 look suspicious, but I see Wd going orange on inbound, too, so I guess that's just bad luck with intermittent oranges.
Wd re-runs fail, too.
JavaScript error: chrome://marionette/content/driver.js, line 1283: TypeError: Services.tm.mainThread.idleDispatch is not a function
looks unrelated to this patch, though.
(In reply to Henri Sivonen (:hsivonen) from comment #26)
> Wd re-runs fail, too.
> JavaScript error: chrome://marionette/content/driver.js, line 1283:
> TypeError: Services.tm.mainThread.idleDispatch is not a function
> looks unrelated to this patch, though.

Looks like there are active landings in that area. Trying rebasing...
Whoa. Do we really not have the script global object set by the time OnStartRequest for file URLs reaches the parser?
(In reply to Henri Sivonen (:hsivonen) from comment #26)

> Wd re-runs fail, too.  JavaScript error:
> chrome://marionette/content/driver.js, line 1283:
> TypeError: Services.tm.mainThread.idleDispatch is not a function looks
> unrelated to this patch, though.

farre renamed Services.tm.mainThread.idleDispatch to
idleDispatchToMainThread in, but several patches landed in
close proximity using the former API.  This will be fixed once
https://bugzilla.mozilla.org/show_bug.cgi?id=1368072 reaches central.
(In reply to Henri Sivonen (:hsivonen) from comment #30)
> Whoa. Do we really not have the script global object set by the time
> OnStartRequest for file URLs reaches the parser?

Filed bug 1376079 and bug 1376080.
And we don't seem to have mDocGroup set for docs loaded via XHR.
billm, r? (I don't know how to properly re-request review after backout on ReviewBoard, so using needinfo.)

Instead of dispatching on the document, which may go away, this patch tries to take the doc group (works for HTTP channels in the non-XHR case; let's treat XHR and non-HTTP as follow-ups) and dispatches unlabeled the same way as nsIDocument would if the doc group was null.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(wmccloskey)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 089160eae8fc -d 05d88e32a4db: rebasing 404002:089160eae8fc "Bug 1333333 - Label runnables in the HTML parser (again). r=billm" (tip)
merging parser/html/nsHtml5StreamParser.cpp
merging parser/html/nsHtml5StreamParser.h
merging parser/html/nsHtml5TreeOpExecutor.cpp
merging parser/html/nsHtml5TreeOperation.cpp
warning: conflicts while merging parser/html/nsHtml5StreamParser.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Filed bug 1376497 about XHR.
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3fc9b909cbb6
Label runnables in the HTML parser (again). r=billm.
> Filed bug 1376079 and bug 1376080.

These were bogus. The real problem is XHR--not the channel type.
https://hg.mozilla.org/mozilla-central/rev/3fc9b909cbb6
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: