Summary: | REGRESSION: (r242181) API test DragAndDropTests.ExternalSourcePlainTextToIFrame is Timing out | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Truitt Savell <tsavell> | ||||
Component: | Tools / Tests | Assignee: | youenn fablet <youennf> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ap, beidson, commit-queue, ggaren, jlewis3, lforschler, ryanhaddad, webkit-bug-importer, wenson_hsieh, youennf | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=195572 | ||||||
Attachments: |
|
Description
Truitt Savell
2019-03-06 10:06:24 PST
What is the next step here? 8 days ago seems like maybe roll back? (In reply to Truitt Savell from comment #0) > > This began timing out with https://trac.webkit.org/changeset/242181/webkit That change caused a *different* assertion, as mentioned in https://bugs.webkit.org/show_bug.cgi?id=195126 Also as mentioned in https://bugs.webkit.org/show_bug.cgi?id=195126, I identified it as a bogus assertion so I removed it in https://trac.webkit.org/changeset/242249/webkit That was 7-8 days ago. The assertion being reported in this bug is a different one that Youenn re-added in https://trac.webkit.org/changeset/242283/webkit (In reply to Alexey Proskuryakov from comment #1) > What is the next step here? 8 days ago seems like maybe roll back? The regression caused 8 days ago was fixed 8 days ago. This assertion was added 6 days ago in https://trac.webkit.org/changeset/242283/webkit I would suggest contacting Youenn about that instead of me. Created attachment 363976 [details]
Patch
Comment on attachment 363976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363976&action=review > Source/WebCore/page/SecurityOrigin.cpp:499 > + ASSERT(!areOriginsMatching(origin1, origin2) || (origin1.toString() == origin2.toString())); This assertion says that matching origins must have identical toString() results. Why is this invariant important? This should be explained in a comment, as otherwise someone will just remove it when it starts to fail after another change. For example, if toString() begins to output file URL host, which it kind of is supposed to do, I'm not sure how it follows that we would have to treat the URLs as not matching - in fact, file://localhost/ is usually same as file://. This assertion alone does very little to explain why a change like the former would be wrong. (In reply to Alexey Proskuryakov from comment #5) > Comment on attachment 363976 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363976&action=review > > > Source/WebCore/page/SecurityOrigin.cpp:499 > > + ASSERT(!areOriginsMatching(origin1, origin2) || (origin1.toString() == origin2.toString())); > > This assertion says that matching origins must have identical toString() > results. Why is this invariant important? This should be explained in a > comment, as otherwise someone will just remove it when it starts to fail > after another change. > > For example, if toString() begins to output file URL host, which it kind of > is supposed to do, I'm not sure how it follows that we would have to treat > the URLs as not matching - in fact, file://localhost/ is usually same as > file://. This assertion alone does very little to explain why a change like > the former would be wrong. Agreed, https://bugs.webkit.org/show_bug.cgi?id=195572 is planning to rename originsMatch to originSerializationsMatch which hopefully makes it clearer. Above bug should also remove some current uses of originsMatch as I believe, SecurityOrigin::isSameOriginAs might be better in most cases. Comment on attachment 363976 [details] Patch Clearing flags on attachment: 363976 Committed r242761: <https://trac.webkit.org/changeset/242761> All reviewed patches have been landed. Closing bug. |