Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2683653004:
Put the BackgroundHTMLParser on oilpan heap (Closed)
|
Created:
3 years, 10 months ago by Charlie Harrison Modified:
3 years, 10 months ago CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, kinuko+watch, loading-reviews+parser_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPut the BackgroundHTMLParser on oilpan heap
This patch puts BackgroundHTMLParser on the oilpan heap, as well as
moving asynchrony logic to the BackgroundHTMLParser.
BUG=689702
Review-Url: https://codereview.chromium.org/2683653004
Cr-Commit-Position: refs/heads/master@{#450559}
Committed: https://chromium.googlesource.com/chromium/src/+/7fe94f0a7f3809fd93916c526d067dcb7f2b4ce3
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : Make BackgroundHTMLParser on oilpan heap #
Total comments: 2
Patch Set 4 : Save ASSERT->DCHECK for another patch #
Total comments: 15
Patch Set 5 : kouhei #Patch Set 6 : remove <memory> #
Messages
Total messages: 36 (25 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Put the BackgroundHTMLParser on oilpan heap BUG= ========== to ========== Put the BackgroundHTMLParser on oilpan heap This patch puts BackgroundHTMLParser on the oilpan heap, as well as moving asynchrony logic to the BackgroundHTMLParser. BUG=689702 ==========
csharrison@chromium.org changed reviewers: + yoav@yoav.ws
Yoav, PTAL? There's a lot more to be done about oilpanning these things, but this is a start. LMK what you think. https://codereview.chromium.org/2683653004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/2683653004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:392: if (!isParsing()) This is necessary because now the bg parser is a member, and doesn't have its destructor called synchronously (and thus its pending tasks killed due to weak ptr semantics).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp (right): https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:28: #include <memory> Do you know why is <memory> included both here and in the h file? I know you just moved it around, but can we get rid of it altogether? https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:195: WTF::passed(std::move(checkpoint)))); Is this switch from sync to async calls related to the oilpan change? https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:50: : public GarbageCollectedFinalized<BackgroundHTMLParser> { I don't think this needs to be Finalized, as it has an empty destructor https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:140: RefPtr<TokenizedChunkQueue> m_tokenizedChunkQueue; Seems like we could have moved TokenizedChunkQueue to be a unique_ptr, right? (as it's created on the HTMlDocumentParser and moved here) https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:392: if (!isParsing()) Could you explain why you added that? Is that related to the Oilpan change?
On 2017/02/09 10:39:00, Yoav Weiss wrote: > https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:392: if > (!isParsing()) > Could you explain why you added that? Is that related to the Oilpan change? nm. Just saw the explanation in your comment above :)
csharrison@chromium.org changed reviewers: + kouhei@chromium.org
+kouhei FYI https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp (right): https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:28: #include <memory> On 2017/02/09 10:39:00, Yoav Weiss wrote: > Do you know why is <memory> included both here and in the h file? I know you > just moved it around, but can we get rid of it altogether? It is for unique_ptr https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:195: WTF::passed(std::move(checkpoint)))); On 2017/02/09 10:39:00, Yoav Weiss wrote: > Is this switch from sync to async calls related to the oilpan change? No, I'm also moving the postTasks from outside the background html parser to within it. If you want, I can revert those changes and go back to the old behavior. https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:50: : public GarbageCollectedFinalized<BackgroundHTMLParser> { On 2017/02/09 10:39:00, Yoav Weiss wrote: > I don't think this needs to be Finalized, as it has an empty destructor It does need to be finalized, because a bunch of fields need finalization (basically anything not oil-panned) https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:140: RefPtr<TokenizedChunkQueue> m_tokenizedChunkQueue; On 2017/02/09 10:39:00, Yoav Weiss wrote: > Seems like we could have moved TokenizedChunkQueue to be a unique_ptr, right? > (as it's created on the HTMlDocumentParser and moved here) We'll probably want this to be a raw pointer, and have the HTMLDocumentParser own the chunk queue. I'm planning on saving that for another patch though.
lgtm myside https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:140: RefPtr<TokenizedChunkQueue> m_tokenizedChunkQueue; On 2017/02/13 22:55:32, Charlie Harrison wrote: > On 2017/02/09 10:39:00, Yoav Weiss wrote: > > Seems like we could have moved TokenizedChunkQueue to be a unique_ptr, right? > > (as it's created on the HTMlDocumentParser and moved here) > > We'll probably want this to be a raw pointer, and have the HTMLDocumentParser > own the chunk queue. I'm planning on saving that for another patch though. Optional (assuming TokenizedChunkQueue to also go away in near future) I should have put more thought on this when introducing tokenizedChunkQueue. Now that m_parser is a Member which is guaranteed to be non-null, I think we can simply reference m_parser->chunkQueue() or something like that instead of keeping a separate ptr.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:140: RefPtr<TokenizedChunkQueue> m_tokenizedChunkQueue; On 2017/02/13 23:09:14, kouhei-atCAM wrote: > On 2017/02/13 22:55:32, Charlie Harrison wrote: > > On 2017/02/09 10:39:00, Yoav Weiss wrote: > > > Seems like we could have moved TokenizedChunkQueue to be a unique_ptr, > right? > > > (as it's created on the HTMlDocumentParser and moved here) > > > > We'll probably want this to be a raw pointer, and have the HTMLDocumentParser > > own the chunk queue. I'm planning on saving that for another patch though. > > Optional (assuming TokenizedChunkQueue to also go away in near future) > I should have put more thought on this when introducing tokenizedChunkQueue. Now > that m_parser is a Member which is guaranteed to be non-null, I think we can > simply reference m_parser->chunkQueue() or something like that instead of > keeping a separate ptr. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2683653004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/2683653004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:392: if (!isParsing()) On 2017/02/08 23:48:01, Charlie Harrison wrote: > This is necessary because now the bg parser is a member, and doesn't have its > destructor called synchronously (and thus its pending tasks killed due to weak > ptr semantics). ok https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp (right): https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:28: #include <memory> On 2017/02/13 22:55:31, Charlie Harrison wrote: > On 2017/02/09 10:39:00, Yoav Weiss wrote: > > Do you know why is <memory> included both here and in the h file? I know you > > just moved it around, but can we get rid of it altogether? > > It is for unique_ptr Yeah, but I'd love to see it *only* in the h, and not re-included in the cpp. Anyway, this is not unique to this patch, and I know some people prefer it this way. I'll argue against it elsewhere :) https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:195: WTF::passed(std::move(checkpoint)))); On 2017/02/13 22:55:32, Charlie Harrison wrote: > On 2017/02/09 10:39:00, Yoav Weiss wrote: > > Is this switch from sync to async calls related to the oilpan change? > > No, I'm also moving the postTasks from outside the background html parser to > within it. If you want, I can revert those changes and go back to the old > behavior. You can leave it as part of this patch https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:50: : public GarbageCollectedFinalized<BackgroundHTMLParser> { On 2017/02/13 22:55:32, Charlie Harrison wrote: > On 2017/02/09 10:39:00, Yoav Weiss wrote: > > I don't think this needs to be Finalized, as it has an empty destructor > > It does need to be finalized, because a bunch of fields need finalization > (basically anything not oil-panned) ok
Thanks, I will do a followup removing the ASSERTS (had to bypass that for this CL). https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp (right): https://codereview.chromium.org/2683653004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:28: #include <memory> On 2017/02/14 06:45:47, Yoav Weiss wrote: > On 2017/02/13 22:55:31, Charlie Harrison wrote: > > On 2017/02/09 10:39:00, Yoav Weiss wrote: > > > Do you know why is <memory> included both here and in the h file? I know you > > > just moved it around, but can we get rid of it altogether? > > > > It is for unique_ptr > > Yeah, but I'd love to see it *only* in the h, and not re-included in the cpp. > Anyway, this is not unique to this patch, and I know some people prefer it this > way. I'll argue against it elsewhere :) Oh, no that is completely fair. I completely misread your comment, apologies. Removed.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org, yoav@yoav.ws Link to the patchset: https://codereview.chromium.org/2683653004/#ps100001 (title: "remove <memory>")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487127613783240,
"parent_rev": "05612a0cb0feafb73ec78c2aebef4c1af797380f", "commit_rev":
"7fe94f0a7f3809fd93916c526d067dcb7f2b4ce3"}
Message was sent while issue was closed.
Description was changed from ========== Put the BackgroundHTMLParser on oilpan heap This patch puts BackgroundHTMLParser on the oilpan heap, as well as moving asynchrony logic to the BackgroundHTMLParser. BUG=689702 ========== to ========== Put the BackgroundHTMLParser on oilpan heap This patch puts BackgroundHTMLParser on the oilpan heap, as well as moving asynchrony logic to the BackgroundHTMLParser. BUG=689702 Review-Url: https://codereview.chromium.org/2683653004 Cr-Commit-Position: refs/heads/master@{#450559} Committed: https://chromium.googlesource.com/chromium/src/+/7fe94f0a7f3809fd93916c526d06... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/7fe94f0a7f3809fd93916c526d06...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2695633007/ by csharrison@chromium.org. The reason for reverting is: Causing frequent crashes: crbug.com/692999 . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
