Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 2 months ago by João Eiras Modified:
6 years ago Reviewers:
abarth-chromium, kouhei (in TOK), eseidel CC:
blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, oystein (OOO til 10th of July) Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionProperly suspend HTMLDocumentParser
When an ExecutionContext was suspended, the HTMLDocumentParser could
still handle parsed chunks, incrementaly build the tree, trigger resource
loads, and build iframes with new clean ExecutionContexts which were not
suspended, therefore triggering ASSERT(m_suspended) inside SuspendableTimer.
Also moved some Document specific code from ScopedPageLoadDeferrer to
FrameLoader, where it logically feels better so it's more related to the
defersLoading state handling, and so it makes the Page to FrameHost
migration easier.
BUG=418116
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186256
Patch Set 1 #Patch Set 2 : Better version of the fix #Patch Set 3 : Fixed wrong method called #
Total comments: 8
Patch Set 4 : Comments #Patch Set 5 : rebase #Patch Set 6 : Added tests #Patch Set 7 : fixed file names #Patch Set 8 : better way to call m_parserScheduler->resume #
Total comments: 5
Patch Set 9 : remote tcs #Patch Set 10 : rebase #
Created: 6 years ago
Messages
Total messages: 43 (15 generated)
joaoe@opera.com changed reviewers: + abarth@chromium.org, eseidel@chromium.org
https://codereview.chromium.org/625583002/diff/40001/Source/core/dom/Executio... File Source/core/dom/ExecutionContext.cpp (right): https://codereview.chromium.org/625583002/diff/40001/Source/core/dom/Executio... Source/core/dom/ExecutionContext.cpp:89: ASSERT(!m_activeDOMObjectsAreSuspended); This matches the asserts that already exist in SuspendableTimer, so it helps finding problems sooner by putting them here. https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/... File Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/... Source/core/html/parser/HTMLDocumentParser.cpp:286: if (m_haveBackgroundParser || !m_speculations.isEmpty()) { I'm a bit unsure about how these two variables relate, hence I modified this check because m_speculations is being added the chunks when the parser is suspended. If you tell me that m_speculations depend on m_haveBackgroundParser being set to true, then I can revert this line. https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/... Source/core/html/parser/HTMLDocumentParser.cpp:293: if (m_tokenizer) As consequence of forcing a call to this method when resumeScheduledTasks is called, m_tokenizer will be empty for document that have done all their parsing. Instead of adding even more state to know if didReceiveParsedChunkFromBackgroundParser was called or not, this is the simplest way I found to avoid the assert inside pumpTokenizer(). https://codereview.chromium.org/625583002/diff/40001/Source/core/page/ScopedP... File Source/core/page/ScopedPageLoadDeferrer.cpp (left): https://codereview.chromium.org/625583002/diff/40001/Source/core/page/ScopedP... Source/core/page/ScopedPageLoadDeferrer.cpp:54: toLocalFrame(frame)->document()->suspendScheduledTasks(); I moved this code into FrameLoader::setDefersLoading() where it feels it belongs better. It will also make the Page to FrameHost migration easier.
Can you add a test case? https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/... File Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/... Source/core/html/parser/HTMLDocumentParser.cpp:286: if (m_haveBackgroundParser || !m_speculations.isEmpty()) { On 2014/10/06 12:14:30, João Eiras wrote: > I'm a bit unsure about how these two variables relate, hence I modified this > check because m_speculations is being added the chunks when the parser is > suspended. If you tell me that m_speculations depend on m_haveBackgroundParser > being set to true, then I can revert this line. Speculations are parsed chunks sent to us from the background parser. Unclear how you could ever have speculations and not have a background parser... https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/... Source/core/html/parser/HTMLDocumentParser.cpp:293: if (m_tokenizer) On 2014/10/06 12:14:31, João Eiras wrote: > As consequence of forcing a call to this method when resumeScheduledTasks is > called, m_tokenizer will be empty for document that have done all their parsing. > Instead of adding even more state to know if > didReceiveParsedChunkFromBackgroundParser was called or not, this is the > simplest way I found to avoid the assert inside pumpTokenizer(). This seems wrong. We should just be more careful as to when we call this method.
On 2014/10/06 18:07:08, eseidel wrote: > Can you add a test case? > Wouldn't know very well how to begin. The original tc is html+js which has an alert() that needs user input (cliking the ok button). AFAIK, putting that tc in LayoutTests won't work because there's no one to click the alert. The alert is there to create the ScopedPageLoadDeferrer. So, Id have to resort to a cpp test but that's going to be complex... lets see what I can do. https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/... File Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/... Source/core/html/parser/HTMLDocumentParser.cpp:286: if (m_haveBackgroundParser || !m_speculations.isEmpty()) { OK, line reverted. https://codereview.chromium.org/625583002/diff/40001/Source/core/html/parser/... Source/core/html/parser/HTMLDocumentParser.cpp:293: if (m_tokenizer) Ok, added a couple extra checks in HTMLDocumentParser::resumeScheduledTasks() then
On 2014/10/07 13:25:07, João Eiras wrote: > On 2014/10/06 18:07:08, eseidel wrote: > > Can you add a test case? > > > Hi Eric. Been busy with other stuff, and thing how to tackle this. Making a c++ testcase to explicitly load chunks of html and somewhere in the middle set the defersLoading flag is an endeavor which I don't think I could accomplish besides being laborious and quite complicated for the sake of a single CL. So I just though of adding a pseudo-alert method to testRunner which emulates what happens in the testcase linked on the bug report, by setting the defersLoading flag for a small interval. What do you think ? Most of the code is in test_runner.cc and is quite straightforward.
On 2014/10/06 18:07:08, eseidel wrote: > Can you add a test case? > I did, finally. I added this extra API to testRunner https://codereview.chromium.org/736723002 https://codereview.chromium.org/725593006 And this bug also has another sibling fix https://codereview.chromium.org/737763002
I've moved off the blink project. Kouhei is a better reviewer for this now.
kouhei@chromium.org changed reviewers: + kouhei@chromium.org
https://codereview.chromium.org/625583002/diff/140001/LayoutTests/fast/parser... File LayoutTests/fast/parser/html-document-parser-suspended-0.html (right): https://codereview.chromium.org/625583002/diff/140001/LayoutTests/fast/parser... LayoutTests/fast/parser/html-document-parser-suspended-0.html:21: <p><iframe src="data:text/html,%3Ch1%3EHi%3C%2Fh1%3E%3Ciframe%20src%3D%22data%3Atext%2Fhtml%2C%3Ch1%3EHi%22%3E%3C%2Fiframe%3E"></iframe></p> Nit: Would you use data:text/html;charset=utf-8, and/or javascript so it is more readable? https://codereview.chromium.org/625583002/diff/140001/LayoutTests/fast/parser... File LayoutTests/fast/parser/html-document-parser-suspended-40.html (right): https://codereview.chromium.org/625583002/diff/140001/LayoutTests/fast/parser... LayoutTests/fast/parser/html-document-parser-suspended-40.html:21: <p><iframe src="data:text/html,%3Ch1%3EHi%3C%2Fh1%3E%3Ciframe%20src%3D%22data%3Atext%2Fhtml%2C%3Ch1%3EHi%22%3E%3C%2Fiframe%3E"></iframe></p> Ditto. https://codereview.chromium.org/625583002/diff/140001/Source/core/html/parser... File Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/625583002/diff/140001/Source/core/html/parser... Source/core/html/parser/HTMLDocumentParser.h:202: bool m_tasksWereSuspended; Would you move this flag under HTMLParserScheduler? I think the flag is more well-fitted there.
On 2014/10/06 18:07:08, eseidel wrote: > Can you add a test case? > After thinking a bit more, and seeing how people are reluctant about the tests I added and the extra testRunner API, I have to say the test is unnecessary because of the extra ASSERTs introduced by this CL. If the old behavior is reintroduced, many of these ASSERTs will trigger. So, can I pretty please, skip the unit test ? It is also quite hard to do a test for this, because it implies suspending the Document, and then checking something does not happen, which will always be dependent on timing, and with timing comes unstable tests.
On 2014/11/21 12:52:35, João Eiras wrote: > On 2014/10/06 18:07:08, eseidel wrote: > > Can you add a test case? > > > > After thinking a bit more, and seeing how people are reluctant about the tests I > added and the extra testRunner API, I have to say the test is unnecessary > because of the extra ASSERTs introduced by this CL. (...) I also thought about changing the Sleep method to a BlockUntilIdle which would call MessageLoop::RunUntilIdle(), but the bug depends on stuff being handled in the html parser thread, so that would not be sufficient.
On 2014/11/24 12:46:23, João Eiras wrote: > On 2014/11/21 12:52:35, João Eiras wrote: > > On 2014/10/06 18:07:08, eseidel wrote: > > > Can you add a test case? > > > > > > > After thinking a bit more, and seeing how people are reluctant about the tests > I > > added and the extra testRunner API, I have to say the test is unnecessary > > because of the extra ASSERTs introduced by this CL. (...) > > I also thought about changing the Sleep method to a BlockUntilIdle which would > call MessageLoop::RunUntilIdle(), but the bug depends on stuff being handled in > the html parser thread, so that would not be sufficient. This codepath will be covered in inspector tests after change in parser scheduling: https://codereview.chromium.org/673603002 So I'm ok with only ASSERTS for now.
> This codepath will be covered in inspector tests after change in parser > scheduling: > https://codereview.chromium.org/673603002 > So I'm ok with only ASSERTS for now. Would you rebase the CL and remove the LayoutTests for now?
On 2014/11/25 06:19:37, kouhei wrote: > > This codepath will be covered in inspector tests after change in parser > > scheduling: Ah, good. I like then to take a peek at those tests. > > https://codereview.chromium.org/673603002 > > So I'm ok with only ASSERTS for now. > Would you rebase the CL and remove the LayoutTests for now? Done.
lgtm
On 2014/11/25 16:01:49, João Eiras wrote: > On 2014/11/25 06:19:37, kouhei wrote: > > > This codepath will be covered in inspector tests after change in parser > > > scheduling: > > Ah, good. I like then to take a peek at those tests. inspector/sources/debugger/ Inspector seems to use suspend for breakpoints in debugger.
https://codereview.chromium.org/625583002/diff/140001/Source/core/html/parser... File Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/625583002/diff/140001/Source/core/html/parser... Source/core/html/parser/HTMLDocumentParser.h:202: bool m_tasksWereSuspended; On 2014/11/19 07:56:10, kouhei wrote: > Would you move this flag under HTMLParserScheduler? I think the flag is more > well-fitted there. Btw, what about this ? Still want me to change it (since you lgtm'ed the review) ?
> This codepath will be covered in inspector tests after change in parser > scheduling: > https://codereview.chromium.org/673603002 > So I'm ok with only ASSERTS for now. I saw that you merge this CL (and got conflicts). Should I still try to commit it ?
On 2014/11/27 01:21:01, João Eiras wrote: > > This codepath will be covered in inspector tests after change in parser > > scheduling: > > https://codereview.chromium.org/673603002 > > So I'm ok with only ASSERTS for now. > > I saw that you merge this CL (and got conflicts). Should I still try to commit > it ? The merge is just for trybot. Please commit this :) https://codereview.chromium.org/625583002/diff/140001/Source/core/html/parser... File Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/625583002/diff/140001/Source/core/html/parser... Source/core/html/parser/HTMLDocumentParser.h:202: bool m_tasksWereSuspended; On 2014/11/27 01:18:38, João Eiras wrote: > On 2014/11/19 07:56:10, kouhei wrote: > > Would you move this flag under HTMLParserScheduler? I think the flag is more > > well-fitted there. > > Btw, what about this ? Still want me to change it (since you lgtm'ed the review) > ? I think we should move the flag, but not necessarily in this CL.
The CQ bit was checked by joaoe@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/625583002/160001
The CQ bit was checked by joaoe@opera.com
The CQ bit was checked by joaoe@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/625583002/160001
The CQ bit was checked by joaoe@opera.com
The CQ bit was checked by joaoe@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/625583002/160001
The CQ bit was checked by joaoe@opera.com
The CQ bit was checked by joaoe@opera.com
The CQ bit was checked by joaoe@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/625583002/160001
The CQ bit was checked by joaoe@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/625583002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/38790)
On 2014/11/27 13:55:48, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win_blink_rel on tryserver.blink > (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/38790) https://codereview.chromium.org/764963002/ should address inspector/sources/debugger/ failures.
On 2014/11/28 02:10:45, kouhei wrote: > On 2014/11/27 13:55:48, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > win_blink_rel on tryserver.blink > > > (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/38790) > > https://codereview.chromium.org/764963002/ should address > inspector/sources/debugger/ failures. Hopefully :) I was debugging the problems last friday but didn't get too far. Lets see how it goes now with a rebase.
On 2014/11/28 02:10:45, kouhei wrote: > On 2014/11/27 13:55:48, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > win_blink_rel on tryserver.blink > > > (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/38790) > > https://codereview.chromium.org/764963002/ should address > inspector/sources/debugger/ failures. Hopefully :) I was debugging the problems last friday but didn't get too far. Lets see how it goes now with a rebase.
The CQ bit was checked by joaoe@opera.com
The CQ bit was unchecked by joaoe@opera.com
The CQ bit was checked by joaoe@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/625583002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://src.chromium.org/viewvc/blink?view=rev&revision=186256 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
