Closed Bug 484121 Opened 11 years ago Closed 11 years ago

Avoid repeatedly passing nsIContentSink parameters to nsIDTD methods

Categories

(Core :: DOM: HTML Parser, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mozilla+ben, Assigned: mozilla+ben)

References

Details

Attachments

(8 files, 7 obsolete files)

14.67 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review
29.07 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review
4.89 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review
8.04 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review
11.01 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review
23.33 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review
37.58 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review
33.93 KB, patch
mrbkap
: review+
mrbkap
: superreview+
Details | Diff | Splinter Review
Attached patch proposed simplification (obsolete) — Splinter Review
The nsIDTD methods BuildModel, DidBuildModel, WillResumeParse, and WillInterruptParse all accept an nsIContentSink* parameter, but as far as I can tell this parameter is always the same as the one passed into WillBuildModel from nsParser, and the DTDs typically ignore it anyway.

I'm experimenting with proxying CNavDTD method calls across threads as part of bug 483010, and that will be simpler and cheaper if I don't have to pass reference-counted objects.
Depends on bug 483939 only because that patch is ready to land and this patch won't apply cleanly without it.  The two patches could be made independent, but there's not much point.
Component: General → HTML: Parser
Product: Firefox → Core
QA Contact: general → parser
Per discussion with mrbkap.
Attachment #368172 - Attachment is obsolete: true
Storing the original nsIContentSink alongside mSink to avoid having to QI back to nsIContentSink.  Assertions enforce identity between mOriginalSink and mSink.
Attachment #368182 - Attachment is obsolete: true
Attachment #368339 - Flags: review?(mrbkap)
Attachment #368339 - Flags: superreview+
Attachment #368339 - Flags: review?(mrbkap)
Attachment #368339 - Flags: review+
Comment on attachment 368339 [details] [diff] [review]
avoiding do_QIs to nsIContentSink

r+sr=mrbkap with the nsRefPtr -> nsCOMPtr change we talked about.
Attached patch excising nsITokenObserver (obsolete) — Splinter Review
Simpler and simpler.
Attachment #368398 - Attachment is obsolete: true
Attachment #368413 - Flags: review+
Comment on attachment 368413 [details] [diff] [review]
catching up with changes to bug 483939 patch

Also s/nsRefPtr/nsCOMPtr/, as requested.
Attachment #368413 - Attachment is patch: true
Attachment #368413 - Attachment mime type: application/octet-stream → text/plain
Blocks: 484869
No longer blocks: 483010
Summary: Simplify nsIDTD interface to avoid repeatedly passing nsIContentSink parameters → Avoid repeatedly passing nsIContentSink parameters to nsIDTD methods
Blocks: 484874
http://hg.mozilla.org/mozilla-central/rev/8f9ff81ef3fa
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Had to back this out to fix orange. Next time, please include bug number and reviewers in the commit message for the patch, so we don't have to go digging for those.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #9)
> Had to back this out to fix orange. Next time, please include bug number and
> reviewers in the commit message for the patch, so we don't have to go digging
> for those.

Thanks for taking care of that.  I would have backed it out from home if I could've.  Lesson learned.
Luckily, the tokenizer creation portion of the original patch was indeed buggy.

We were using a DTD to create a tokenizer before telling the DTD (via WillBuildModel) what sink it would be using.  But we couldn't call WillBuildModel without a tokenizer, so something had to give.

What gave: I moved the tokenizer flag computation into CParserContext.
Attachment #371740 - Attachment is obsolete: true
Attachment #371956 - Flags: review?(mrbkap)
Comment on attachment 371956 [details] [diff] [review]
fixing tokenizer flag inconsistency bug

I wonder why neither of us could reproduce the crash...
Attachment #371956 - Flags: superreview+
Attachment #371956 - Flags: review?(mrbkap)
Attachment #371956 - Flags: review+
Together with the previous patch, this patch implements the original simplifications.
Attachment #368339 - Attachment is obsolete: true
Attachment #368413 - Attachment is obsolete: true
Attachment #371962 - Flags: review?(mrbkap)
Attachment #371962 - Flags: superreview+
Attachment #371962 - Flags: review?(mrbkap)
Attachment #371962 - Flags: review+
Gross, the tokenizer creation patch causes all kinds of mochitest failures with this test:
--test-path=parser/htmlparser/tests/mochitest/test_html5_tree_construction.html

Looking into it...
(In reply to comment #15)
> Gross, the tokenizer creation patch causes all kinds of mochitest failures with
> this test:
> --test-path=parser/htmlparser/tests/mochitest/test_html5_tree_construction.html
> 
> Looking into it...

The problem is that the other nsHTMLTokenizer constructor arguments also are not passed through from the parser context to the DTD.  Putting this straight is going to take some thought.
Letting DTDs create tokenizers proved more error-prone than expected, so I'm abandoning that strategy in this six-part revamp.

This first patch addresses one of the original motivations by consolidating all the computations of tokenizer flags into a single static method, nsHTMLTokenizer::GetFlags(nsIContentSink*).
Funny that neither of the proposed alternatives were necessary.

Also, this patch adds an nsDTDMode parameter to the nsIContentSink::WillBuildModel signature.  This avoids the sink calling back into the parser to determine what mode the DTD wanted to use (WTFFF).
So much simpler to simply call mSink->WillResume() and mSink->WillInterrupt() in the parser. Simple!
Attachment #373028 - Attachment description: revamp part 2/6: get rid of nsIDTD::Will{Resume,Interrupt}Parse → revamp part 3/6: get rid of nsIDTD::Will{Resume,Interrupt}Parse
This change is advisable because it strictly enforces the calling of nsIContentSink::{Will,Did}BuildModel regardless of whether nsIDTD::{Will,Did}BuildModel succeeded.

It became necessary because the DTD may not always have the same sink as the parser (e.g., if WillBuildModel was never called).  There's probably another bug lurking here, but for now I'd rather just program defensively.
This patch accomplishes something very similar to the original patch for this bug.  Mrbkap will find it pretty familiar.

Also, removing HandleToken from the nsIDTD public interface.
A DTD shouldn't care about the parser that calls its methods.  With this patch, nsIParsers are no longer passed to nsIDTD methods, and parser references are no longer stored by the DTDs.
Lesson learned: when adding a parameter to an interface method, it's a bad idea to make the parameter optional.
Attachment #373027 - Attachment is obsolete: true
Been running these patches through the try server periodically to determine if I'm responsible for any of the warnings.

Here's the most recent run:

https://build.mozilla.org/tryserver-builds/bnewman@mozilla.com-try-bb5b452bb5f/

This run produced two warnings:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1243990378.1243997915.8032.gz&fulltext=1

Both of these warnings already have bugs filed against them:

https://bugzilla.mozilla.org/show_bug.cgi?id=487750
https://bugzilla.mozilla.org/show_bug.cgi?id=494749

Given this evidence, I feel confident requesting review.  Since a couple of the patches have changed in minor ways due to upkeep, I could repost them here, but I don't want to clutter the bug.
Attachment #373026 - Flags: review?(mrbkap)
Attachment #373959 - Flags: review?(mrbkap)
Attachment #373028 - Flags: review?(mrbkap)
Attachment #373029 - Flags: review?(mrbkap)
Attachment #373030 - Flags: review?(mrbkap)
Attachment #373032 - Flags: review?(mrbkap)
Attachment #373026 - Flags: superreview+
Attachment #373026 - Flags: review?(mrbkap)
Attachment #373026 - Flags: review+
Attachment #373028 - Flags: superreview+
Attachment #373028 - Flags: review?(mrbkap)
Attachment #373028 - Flags: review+
Comment on attachment 373029 [details] [diff] [review]
revamp part 4/6: let the parser call nsIContentSink::{Will,Did}BuildModel directly

>+        nsresult dtdResult =  mDTD->DidBuildModel(anErrorCode,this,mSink),
>+                sinkResult = mSink->DidBuildModel();
>+        result = NS_FAILED(sinkResult) ? sinkResult : dtdResult;

Is it worth making this pattern a private method on nsParser with a comment about why we do this? r+sr=mrbkap with that addressed.
Attachment #373029 - Flags: superreview+
Attachment #373029 - Flags: review?(mrbkap)
Attachment #373029 - Flags: review+
Attachment #373030 - Flags: superreview+
Attachment #373030 - Flags: review?(mrbkap)
Attachment #373030 - Flags: review+
Comment on attachment 373030 [details] [diff] [review]
revamp part 5/6: avoid repeatedly passing nsIContentSink parameters to nsIDTD methods

I guess it's not worth filing a bug on switching the DTDs away from NS_IMETHOD{,IMP} at this point...
Comment on attachment 373032 [details] [diff] [review]
revamp part 6/6: don't let DTDs hold parser references

>@@ -710,16 +699,18 @@ nsresult CViewSourceHTML::WriteTag(PRInt
>   nsresult result=NS_OK;
> 
>   // adjust line number to what it will be after we finish writing this tag
>   // XXXbz life here sucks.  We can't use the GetNewlineCount on the token,
>   // because Text tokens in <style>, <script>, etc lie through their teeth.
>   // On the other hand, the parser messes up newline counting in some token
>   // types (bug 137315).  So our line numbers will disagree with the parser's
>   // in some cases...
>+  // XXXbenjamn Shouldn't we be paying attention to the aCountLines BuildModel
>+  // parameter here?

Technically, I suppose so. However, since view source doesn't run scripts (and therefore doesn't have to worry about document.write) it doesn't matter.
Attachment #373032 - Flags: superreview+
Attachment #373032 - Flags: review?(mrbkap)
Attachment #373032 - Flags: review+
Attachment #373959 - Flags: superreview+
Attachment #373959 - Flags: review?(mrbkap)
Attachment #373959 - Flags: review+
Comment on attachment 373959 [details] [diff] [review]
small change to revamp part 2/6

>+NS_IMETHODIMP_(nsDTDMode)
>+CViewSourceHTML::GetMode() const
>+{
>+  return eDTDMode_full_standards; // lie, lie, lie

Expand on this comment a little more? It's kind of subtle.
Duplicate of this bug: 484874
You need to log in before you can comment on or make changes to this bug.