Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1642563002:
Introduce EventSourceParser (Closed)
|
Created:
4 years, 10 months ago by yhirano Modified:
4 years, 10 months ago CC:
chromium-reviews, blink-reviews, blink-reviews-events_chromium.org, dglazkov+blink, eae+blinkwatch Base URL:
https://chromium.googlesource.com/chromium/src.git@event-source-retry-fix Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce EventSourceParser
This change the parsing functionality from EventSource class. This change
improves the performance for long messages. In my environment, it reduces
the time taken for receiving a 16m long message from over 30s to under 2s.
This CL does not change the behavior, except one point. The spec says
the last event ID string should be updated when there is a valid id
field in an event even when the event is not dispatched because
the data buffer is empty. The previous implementation was wrong on this
point and this CL fixes it.
BUG=570148
Committed: https://crrev.com/f8259072812989d0042d6c287c03a9a351e2dfff
Cr-Commit-Position: refs/heads/master@{#376496}
Patch Set 1 : #
Total comments: 24
Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 10
Patch Set 5 : #
Total comments: 8
Patch Set 6 : rebase #Patch Set 7 : #Patch Set 8 : #
Total comments: 1
Messages
Total messages: 47 (22 generated)
Description was changed from ========== Introduce EventSourceParser This change the parsing functionality from EventSource class. This change improves the performance for long messages. In my environment, it reduces the time taken for receiving a 16m long message from over 30s to less than 2s. BUG=570148 ========== to ========== Introduce EventSourceParser This change the parsing functionality from EventSource class. This change improves the performance for long messages. In my environment, it reduces the time taken for receiving a 16m long message from over 30s to under 2s. BUG=570148 ==========
Patchset #3 (id:40001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:100001) has been deleted
yhirano@chromium.org changed reviewers: + tyoshino@chromium.org
https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSource.h (right): https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSource.h:42: #include "wtf/RefPtr.h" add wtf/Forward.h for AtomicString https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParser.cpp (right): https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:29: size_t start = 0; explain that start is the start position of non-BOM bytes in |bytes| which are not yet processed. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:31: for (size_t i = 0; i < size && !m_isStopped; ++i) { explain that kBOM doesn't contain either CL or LF and therefore we can scan BOM and line in parallel. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:75: m_event = AtomicString(); use nullAtom? https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:106: auto reconnectionTime = fromUTF8(m_line.data() + fieldValueStart, fieldValueSize).toUInt64(&ok); can we use charactersToUInt64() directly? https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParser.h (right): https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.h:12: #include "wtf/text/WTFString.h" Forward.h is enough? https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.h:41: Vector<char> m_line; blank line here https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.h:42: AtomicString m_event; how about m_eventType? https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.h:44: // This is used to store the currently processed ID field value. as you've chosen not to reset m_id unlike the code before this CL, this comment is not correct. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.h:47: AtomicString m_lastEventId; blank line here https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.h:48: Member<Client> m_client; blank line here https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParserTest.cpp (right): https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParserTest.cpp:293: } Please add BOM test that uses enqueueOneByOne() https://codereview.chromium.org/1642563002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParser.cpp (right): https://codereview.chromium.org/1642563002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:22: return newTextCodec(UTF8Encoding())->decode(bytes, size, WTF::DataEOF); hmm, you've chosen this approach since TextCodec is not resettable?
Patchset #5 (id:200001) has been deleted
Patchset #4 (id:180001) has been deleted
Description was changed from ========== Introduce EventSourceParser This change the parsing functionality from EventSource class. This change improves the performance for long messages. In my environment, it reduces the time taken for receiving a 16m long message from over 30s to under 2s. BUG=570148 ========== to ========== Introduce EventSourceParser This change the parsing functionality from EventSource class. This change improves the performance for long messages. In my environment, it reduces the time taken for receiving a 16m long message from over 30s to under 2s. This CL does not change the behavior, except one point. The spec says the last event ID string should be updated when there is a valid id field in an event even when the event is not dispatched because the data buffer is empty. The previous implementation was wrong on this point and this CL fixes it. BUG=570148 ==========
https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSource.h (right): https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSource.h:42: #include "wtf/RefPtr.h" On 2016/01/28 07:31:43, tyoshino wrote: > add wtf/Forward.h for AtomicString Done. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParser.cpp (right): https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:29: size_t start = 0; On 2016/01/28 07:31:43, tyoshino wrote: > explain that start is the start position of non-BOM bytes in |bytes| which are > not yet processed. Done. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:31: for (size_t i = 0; i < size && !m_isStopped; ++i) { On 2016/01/28 07:31:43, tyoshino wrote: > explain that kBOM doesn't contain either CL or LF and therefore we can scan BOM > and line in parallel. Done. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:75: m_event = AtomicString(); On 2016/01/28 07:31:43, tyoshino wrote: > use nullAtom? Done. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:106: auto reconnectionTime = fromUTF8(m_line.data() + fieldValueStart, fieldValueSize).toUInt64(&ok); On 2016/01/28 07:31:43, tyoshino wrote: > can we use charactersToUInt64() directly? charactersToUint64 expects LChar* or UChar* and we have char*. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParser.h (right): https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.h:12: #include "wtf/text/WTFString.h" On 2016/01/28 07:31:43, tyoshino wrote: > Forward.h is enough? I keep the header because I've just added |fromUTF8|. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.h:41: Vector<char> m_line; On 2016/01/28 07:31:43, tyoshino wrote: > blank line here Done. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.h:42: AtomicString m_event; On 2016/01/28 07:31:43, tyoshino wrote: > how about m_eventType? Done. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.h:44: // This is used to store the currently processed ID field value. On 2016/01/28 07:31:43, tyoshino wrote: > as you've chosen not to reset m_id unlike the code before this CL, this comment > is not correct. Updated the comment. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.h:47: AtomicString m_lastEventId; On 2016/01/28 07:31:43, tyoshino wrote: > blank line here Done. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.h:48: Member<Client> m_client; On 2016/01/28 07:31:44, tyoshino wrote: > blank line here Done. https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParserTest.cpp (right): https://codereview.chromium.org/1642563002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParserTest.cpp:293: } On 2016/01/28 07:31:44, tyoshino wrote: > Please add BOM test that uses enqueueOneByOne() Done. https://codereview.chromium.org/1642563002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParser.cpp (right): https://codereview.chromium.org/1642563002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:22: return newTextCodec(UTF8Encoding())->decode(bytes, size, WTF::DataEOF); On 2016/01/28 07:31:44, tyoshino wrote: > hmm, you've chosen this approach since TextCodec is not resettable? Changed so that the parser keeps the codec instance.
lgtm https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSource.cpp (right): https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSource.cpp:314: void EventSource::onMessageEvent(const AtomicString& event, const String& data, const AtomicString& lastEventId) eventType https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSource.cpp:319: InspectorInstrumentation::willDispachEventSourceEvent(executionContext(), this, event, lastEventId, data.charactersWithNullTermination()); charactersWithNullTermination() appends \0 to the end but the old code didn't? anyway, how about changing InspectorResourceAgent::willDispachEventSourceEvent() to accept a String? It's converting Vector<UChar> to a String again. https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParser.cpp (right): https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:8: #include "core/page/EventSource.h" needed? https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:13: #include "wtf/text/TextEncodingRegistry.h" add wtf/Assertions.h https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParserTest.cpp (right): https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParserTest.cpp:7: #include "core/page/EventSource.h" unnecessary?
https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSource.cpp (right): https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSource.cpp:314: void EventSource::onMessageEvent(const AtomicString& event, const String& data, const AtomicString& lastEventId) On 2016/01/29 07:00:14, tyoshino wrote: > eventType Done. https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSource.cpp:319: InspectorInstrumentation::willDispachEventSourceEvent(executionContext(), this, event, lastEventId, data.charactersWithNullTermination()); On 2016/01/29 07:00:14, tyoshino wrote: > charactersWithNullTermination() appends \0 to the end but the old code didn't? > > anyway, how about changing InspectorResourceAgent::willDispachEventSourceEvent() > to accept a String? It's converting Vector<UChar> to a String again. Done. https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParser.cpp (right): https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:8: #include "core/page/EventSource.h" On 2016/01/29 07:00:14, tyoshino wrote: > needed? For EventSource::defaultReconnectDelay. https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:13: #include "wtf/text/TextEncodingRegistry.h" On 2016/01/29 07:00:14, tyoshino wrote: > add wtf/Assertions.h Done. https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParserTest.cpp (right): https://codereview.chromium.org/1642563002/diff/210001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParserTest.cpp:7: #include "core/page/EventSource.h" On 2016/01/29 07:00:15, tyoshino wrote: > unnecessary? For EventSource::defaultReconnectDelay.
yhirano@chromium.org changed reviewers: + yurys@chromium.org
+yurys for OWNER review.
On 2016/01/29 11:41:49, yhirano wrote: > +yurys for OWNER review. ping
yurys@chromium.org changed reviewers: + dgozman@chromium.org - yurys@chromium.org
yurys@chromium.org changed reviewers: + yurys@chromium.org
+dgozman
dgozman@chromium.org changed reviewers: + pfeldman@chromium.org
Sorry, I'm not a owner of core/page. Over to pfeldman.
ping
https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParser.cpp (right): https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:31: const unsigned char kBOM[] = {0xef, 0xbb, 0xbf}; Do you want to run this by security team since it is working with the page-provided data in a non-trivial manner?
yhirano@chromium.org changed reviewers: + tsepez@chromium.org
On 2016/02/11 18:54:41, pfeldman wrote: > https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/page/EventSourceParser.cpp (right): > > https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/page/EventSourceParser.cpp:31: const unsigned > char kBOM[] = {0xef, 0xbb, 0xbf}; > Do you want to run this by security team since it is working with the > page-provided data in a non-trivial manner? +tsepez@. Tom, can you take a look?
https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSource.cpp (right): https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSource.cpp:131: request.setHTTPHeaderField(HTTPNames::Last_Event_ID, AtomicString(reinterpret_cast<const LChar*>(lastEventIdUtf8.data()), lastEventIdUtf8.length())); So if this is untrusted, do we need to verfiy that it doesn't contain \r\n, etc, which would lead to header injection? https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParser.cpp (right): https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:31: const unsigned char kBOM[] = {0xef, 0xbb, 0xbf}; On 2016/02/11 18:54:41, pfeldman wrote: > Do you want to run this by security team since it is working with the > page-provided data in a non-trivial manner? The array indexing in here looks ok, what you do with the untrusted values returned by parse would be the only concern.
https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSource.cpp (right): https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSource.cpp:131: request.setHTTPHeaderField(HTTPNames::Last_Event_ID, AtomicString(reinterpret_cast<const LChar*>(lastEventIdUtf8.data()), lastEventIdUtf8.length())); On 2016/02/12 17:14:30, Tom Sepez wrote: > So if this is untrusted, do we need to verfiy that it doesn't contain \r\n, etc, > which would lead to header injection? It cannot contain CR or LF, but it can contain other control characters. I think it's a spec problem: I filed https://github.com/whatwg/html/issues/689 and am discussing.
> It cannot contain CR or LF, but it can contain other control characters. I think > it's a spec problem: I filed https://github.com/whatwg/html/issues/689 and am > discussing. OK, thanks, LGTM pending the resolution of that issue.
yhirano@chromium.org changed reviewers: + kinuko@chromium.org
+kinuko
lgtm https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParser.cpp (right): https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:96: } else if (fieldName == "data") { nit: maybe return at the end of each if and make "else if" to just "if" https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParserTest.cpp (right): https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParserTest.cpp:252: // the implementation or the spec. nit: if you have a spec bug or crbug please link to it.
https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParser.cpp (right): https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParser.cpp:96: } else if (fieldName == "data") { On 2016/02/18 09:15:00, kinuko wrote: > nit: maybe return at the end of each if and make "else if" to just "if" Done. https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParserTest.cpp (right): https://codereview.chromium.org/1642563002/diff/230001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParserTest.cpp:252: // the implementation or the spec. On 2016/02/18 09:15:00, kinuko wrote: > nit: if you have a spec bug or crbug please link to it. Done.
The CQ bit was checked by yhirano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tyoshino@chromium.org, kinuko@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/1642563002/#ps290001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1642563002/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1642563002/290001
Message was sent while issue was closed.
Description was changed from ========== Introduce EventSourceParser This change the parsing functionality from EventSource class. This change improves the performance for long messages. In my environment, it reduces the time taken for receiving a 16m long message from over 30s to under 2s. This CL does not change the behavior, except one point. The spec says the last event ID string should be updated when there is a valid id field in an event even when the event is not dispatched because the data buffer is empty. The previous implementation was wrong on this point and this CL fixes it. BUG=570148 ========== to ========== Introduce EventSourceParser This change the parsing functionality from EventSource class. This change improves the performance for long messages. In my environment, it reduces the time taken for receiving a 16m long message from over 30s to under 2s. This CL does not change the behavior, except one point. The spec says the last event ID string should be updated when there is a valid id field in an event even when the event is not dispatched because the data buffer is empty. The previous implementation was wrong on this point and this CL fixes it. BUG=570148 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:290001)
Message was sent while issue was closed.
Description was changed from ========== Introduce EventSourceParser This change the parsing functionality from EventSource class. This change improves the performance for long messages. In my environment, it reduces the time taken for receiving a 16m long message from over 30s to under 2s. This CL does not change the behavior, except one point. The spec says the last event ID string should be updated when there is a valid id field in an event even when the event is not dispatched because the data buffer is empty. The previous implementation was wrong on this point and this CL fixes it. BUG=570148 ========== to ========== Introduce EventSourceParser This change the parsing functionality from EventSource class. This change improves the performance for long messages. In my environment, it reduces the time taken for receiving a 16m long message from over 30s to under 2s. This CL does not change the behavior, except one point. The spec says the last event ID string should be updated when there is a valid id field in an event even when the event is not dispatched because the data buffer is empty. The previous implementation was wrong on this point and this CL fixes it. BUG=570148 Committed: https://crrev.com/f8259072812989d0042d6c287c03a9a351e2dfff Cr-Commit-Position: refs/heads/master@{#376496} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f8259072812989d0042d6c287c03a9a351e2dfff Cr-Commit-Position: refs/heads/master@{#376496}
Message was sent while issue was closed.
krasin@chromium.org changed reviewers: + krasin@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1642563002/diff/290001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/EventSourceParserTest.cpp (right): https://codereview.chromium.org/1642563002/diff/290001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/EventSourceParserTest.cpp:387: TEST(EventSourceParserStoppingTest, StopWhileParsing) Early warning. This test broke CFI buildbot: https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/4276 Locally, I see the reason: ../../third_party/WebKit/Source/platform/heap/GCInfo.h:37:9: runtime error: control flow integrity check for type 'blink::(anonymous namespace)::Client' failed during cast to unrelated type (vtable address 0x0000036efd50) 0x0000036efd50: note: vtable is of type 'blink::(anonymous namespace)::StoppingClient' a more comprehensive bug report is to follow.
Message was sent while issue was closed.
On 2016/02/19 22:54:37, krasin1 wrote: > https://codereview.chromium.org/1642563002/diff/290001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/page/EventSourceParserTest.cpp (right): > > https://codereview.chromium.org/1642563002/diff/290001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/page/EventSourceParserTest.cpp:387: > TEST(EventSourceParserStoppingTest, StopWhileParsing) > Early warning. This test broke CFI buildbot: > https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/4276 > > Locally, I see the reason: > ../../third_party/WebKit/Source/platform/heap/GCInfo.h:37:9: runtime error: > control flow integrity check for type 'blink::(anonymous namespace)::Client' > failed during cast to unrelated type (vtable address 0x0000036efd50) > 0x0000036efd50: note: vtable is of type 'blink::(anonymous > namespace)::StoppingClient' > > a more comprehensive bug report is to follow. Trying to build Chrome with the cfi setting.
Message was sent while issue was closed.
On 2016/02/19 23:05:03, yhirano wrote: > On 2016/02/19 22:54:37, krasin1 wrote: > > > https://codereview.chromium.org/1642563002/diff/290001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/page/EventSourceParserTest.cpp (right): > > > > > https://codereview.chromium.org/1642563002/diff/290001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/page/EventSourceParserTest.cpp:387: > > TEST(EventSourceParserStoppingTest, StopWhileParsing) > > Early warning. This test broke CFI buildbot: > > https://build.chromium.org/p/chromium.fyi/builders/CFI%20Linux/builds/4276 > > > > Locally, I see the reason: > > ../../third_party/WebKit/Source/platform/heap/GCInfo.h:37:9: runtime error: > > control flow integrity check for type 'blink::(anonymous namespace)::Client' > > failed during cast to unrelated type (vtable address 0x0000036efd50) > > 0x0000036efd50: note: vtable is of type 'blink::(anonymous > > namespace)::StoppingClient' > > > > a more comprehensive bug report is to follow. > > Trying to build Chrome with the cfi setting. I haven't succeeded to build, but I found a bad cast: https://codereview.chromium.org/1720493002/
Message was sent while issue was closed.
> a more comprehensive bug report is to follow. Bug filed: https://crbug.com/588320 Fix proposed: https://codereview.chromium.org/1717863002/ |
