Skip to content

[EWS] Report basic statuses to GitHub#72

Merged
JonWBedard merged 0 commit into
WebKit:mainfrom
JonWBedard:eng/github-ews-report-statuses
Jan 19, 2022
Merged

[EWS] Report basic statuses to GitHub#72
JonWBedard merged 0 commit into
WebKit:mainfrom
JonWBedard:eng/github-ews-report-statuses

Conversation

@JonWBedard

@JonWBedard JonWBedard commented Jan 14, 2022

Copy link
Copy Markdown
Member

76b8712

[EWS] Report basic statuses to GitHub
https://bugs.webkit.org/show_bug.cgi?id=235210
<rdar://problem/87576183 >

Reviewed by Aakash Jain.

* Tools/CISupport/ews-build/events.py:
(Events.sendDataToEWS): Renamed from sendData.
(Events.sendDataToGitHub): Post status for a commit to GitHub.
(Events.extractProperty): We need to extract more than patchID from
build properties.
(Events.buildFinishedGitHub): Report the pass/failed state of a build to GitHub.
(Events.buildFinished): Every time a build finishes, send data to GitHub.
(Events.stepStartedGitHub): Update GitHub with the current step being run,
and a failure status if that's already known.
(Events.stepStarted): Every time a new step starts, send data to GitHub.
(Events.stepFinished):
(Events.sendData): Deleted.
(Events.getPatchID): Deleted.
* Tools/CISupport/ews-build/steps.py:
(GitHub.commit_status_url): Construct a status url for a commit in a repository.


Canonical link: https://commits.webkit.org/246199@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@288246 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@JonWBedard JonWBedard self-assigned this Jan 14, 2022
@JonWBedard
JonWBedard requested a review from aj062 January 14, 2022 00:25
Comment thread Tools/CISupport/ews-build/events.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this method name intentional? (same as buildbot method name)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, although I can't say I gave much thought to the name. It could be anything that gets the idea of extracting a property from the build dictionary across.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same name might cause confusion or might be misleading. We can call it something like extractProperty. I guess then we can also rename property_ to property.

@JonWBedard
JonWBedard requested a review from aj062 January 14, 2022 17:17
@JonWBedard

JonWBedard commented Jan 14, 2022

Copy link
Copy Markdown
Member Author

Just proto-typing EWS comment format (which isn't something this pull-request implements):

EWS Result summary

✅ 🧪 style   ⏳ 🧪 webkitpy  ✅ 🛠 ios   ✅ 🛠 ios-sim  ✅ 🛠 mac
✅ 🛠 mac-debug  ✅ 🛠 mac-AS-debug✅ 🛠 tv   ✅ 🛠 tv-sim  ✅ 🛠 watch
✅ 🛠 watch-sim  ✅ 🛠 gtk   ✅ 🛠 wpe   ✅ 🛠 wincairo  ❌ 🛠🧪 win
✅ 🧪 ios-wk2  ❌ 🧪 mac-wk1  ❌ 🧪 mac-wk2  ❌ 🧪 mac-debug-wk1❌ 🧪 mac-AS-debug-wk2
✅ 🧪 api-ios  ✅ 🧪 api-mac  ✅ 🧪 api-gtk  ✅ 🧪 bindings  ✅ 🧪 webkitperl
⏳ 🧪 services  

Details

❌ 🛠🧪 win

Found 1 new test failure
 http/tests/websocket/tests/hybi/simple-wss.html

❌ 🧪 mac-wk1

Found 5 new test failures
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...

❌ 🧪 mac-wk2

Found 5 new test failures
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...

❌ 🧪 mac-debug-wk1

Found 5 new test failures
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...

❌ 🧪 mac-AS-debug-wk2

Found 5 new test failures
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...
 fast/overflow/inline-block-scroll-overflow.html...

@aj062

aj062 commented Jan 14, 2022

Copy link
Copy Markdown
Member

Test editing a comment one more time!

@JonWBedard
JonWBedard force-pushed the eng/github-ews-report-statuses branch from f67f0bd to e4729fc Compare January 14, 2022 21:06
Comment thread Tools/CISupport/ews-build/events.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better to construct all the urls at a single place, inside the GitHub class.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How exactly would we do that though? We have at least two different URL paths that we're using at the moment, there will likely be 3 or 4 by the time we're done with GitHub stuff in EWS. I suppose we could pass a "path" argument to the api_url function, that would look like this:
d = agent.request(b'POST', GitHub.api_url(repository, 'statuses/{}'.format(sha)), sha).encode('utf-8'))...
which doesn't seem any better, possibly more confusing because you would need to consult the function to see what the extra argument is doing.

Comment thread Tools/CISupport/ews-build/events.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Since this is pretty long, it might be a good idea to store in a variable for better readability.

Comment thread Tools/CISupport/ews-build/events.py Outdated
Comment thread Tools/CISupport/ews-build/events.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same name might cause confusion or might be misleading. We can call it something like extractProperty. I guess then we can also rename property_ to property.

Comment thread Tools/CISupport/ews-build/events.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all these events, it might be better/cleaner to create two methods one for ews and one for github. Maybe something like: sendBuildStartedDataToEWS, sendBuildStartedDataToGitHub(), and have buildStarted() call them appropriately.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that apply only to BuildStarted or to all of them? I was trying to avoid a ton of function bloat in this class, if we did this everywhere, we'd be changing 4 functions to into 12.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I just removed the code in BuildStarted completely. The reason I did this is that we really don't want to report anything until we validate the patch is actually relevant, I haven't made that change in it's entirety yet, but given we're reporting on each intermediate step, it shouldn't be terribly difficult to do.

@aj062

aj062 commented Jan 18, 2022

Copy link
Copy Markdown
Member

@JonWBedard have you looked into buildbot implementation GitHubStatusPush http://docs.buildbot.net/2.8.3/manual/configuration/reporters.html#githubstatuspush

If that works well, it might be better to use that instead of maintaining our own code, for something which has already been developed by buildbot and hopefully polished over a period of time.

Similarly, there is also GitHubCommentPush which might be helpful with commenting on GitHub.

@JonWBedard

JonWBedard commented Jan 18, 2022

Copy link
Copy Markdown
Member Author

@JonWBedard have you looked into buildbot implementation GitHubStatusPush http://docs.buildbot.net/2.8.3/manual/configuration/reporters.html#githubstatuspush

If that works well, it might be better to use that instead of maintaining our own code, for something which has already been developed by buildbot and hopefully polished over a period of time.

Similarly, there is also GitHubCommentPush which might be helpful with commenting on GitHub.

I did look into this, and I think we want the finer control of handling all this ourselves. As an example of something we're probably going to want to do: we probably don't want to report to GitHub that a build has started until we've passed the validation phase, otherwise we'll end up reporting "success" for builds that actually didn't run.

Our comments are almost certainly going to be coming from ews.webkit.org, not ewe-build.webkit.org because one comment will contain details for multiple queues. That being said, if we want to make explicit comments on test failures, yes, we should use GitHubCommentPush

@JonWBedard
JonWBedard force-pushed the eng/github-ews-report-statuses branch from e4729fc to e1a7379 Compare January 18, 2022 16:30
@JonWBedard
JonWBedard requested a review from aj062 January 18, 2022 16:36
@JonWBedard
JonWBedard force-pushed the eng/github-ews-report-statuses branch 4 times, most recently from 43440b1 to caa2937 Compare January 19, 2022 22:13

@aj062 aj062 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, this change is temporary for quick prototyping. Since we want to generate comments from ews django app, eventually ews-app would be the one sending data to github, not buildbot. Buildbot will send the data (for both patch and pr work-flow) to ews-app.

So we would want to revert/remove this change when we implement that communication with ews-app. So, can you please split this patch in two parts/commits, first one which would be permanent change, and second one which could be reverted easily, and then land as two separate commits.

Comment thread Tools/CISupport/ews-build/events.py Outdated
Comment thread Tools/CISupport/ews-build/events.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a good idea to print something to logs here, in case we have to debug a scenario when there is a problem with github communication.

Comment thread Tools/CISupport/ews-build/events.py Outdated
Comment thread Tools/CISupport/ews-build/events.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we using 'github.number' as the main identifier to decide whether it is a PR or a patch based build? Whatever we are using, let's make sure to use the same identifier through-out the ews code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I've been keying off of. I often check more that just github.number, but I try to make that the primary check.

Comment thread Tools/CISupport/ews-build/events.py Outdated
Comment thread Tools/CISupport/ews-build/events.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even need this check? We didn't had it before. If we don't have it, it will try to send data to ews django app, and django app would reject it. I guess that might log an error in logs (i haven't checked though).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also didn't have tons of builds without patch_id before, though. Which is why I think we should have keep this check.

Comment thread Tools/CISupport/ews-build/events.py Outdated
Comment thread Tools/CISupport/ews-build/events.py Outdated
Comment thread Tools/CISupport/ews-build/events.py Outdated
Comment thread Tools/CISupport/ews-build/events.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's extra database call, but seems like we can't avoid that.

@JonWBedard
JonWBedard force-pushed the eng/github-ews-report-statuses branch from caa2937 to 2975d2c Compare January 19, 2022 23:20
@JonWBedard JonWBedard closed this Jan 19, 2022
@JonWBedard
JonWBedard force-pushed the eng/github-ews-report-statuses branch from 2975d2c to 76b8712 Compare January 19, 2022 23:28
@JonWBedard
JonWBedard merged commit 76b8712 into WebKit:main Jan 19, 2022
@JonWBedard

Copy link
Copy Markdown
Member Author

@JonWBedard
JonWBedard deleted the eng/github-ews-report-statuses branch January 19, 2022 23:28
webkit-early-warning-system pushed a commit to MenloDorian/Webkit that referenced this pull request Jun 23, 2022
https://bugs.webkit.org/show_bug.cgi?id=241905

Reviewed by Yusuke Suzuki.

offlineasm used to emit this LLInt code:
    ".loc 1 996\n"        "ldr x19, [x0] \n"                   // LowLevelInterpreter.asm:996
    ".loc 1 997\n"        "ldr x20, [x0, WebKit#8] \n"               // LowLevelInterpreter.asm:997
    ".loc 1 998\n"        "ldr x21, [x0, WebKit#16] \n"              // LowLevelInterpreter.asm:998
    ".loc 1 999\n"        "ldr x22, [x0, WebKit#24] \n"              // LowLevelInterpreter.asm:999
    ...
    ".loc 1 1006\n"       "ldr d8, [x0, WebKit#80] \n"               // LowLevelInterpreter.asm:1006
    ".loc 1 1007\n"       "ldr d9, [x0, WebKit#88] \n"               // LowLevelInterpreter.asm:1007
    ".loc 1 1008\n"       "ldr d10, [x0, WebKit#96] \n"              // LowLevelInterpreter.asm:1008
    ".loc 1 1009\n"       "ldr d11, [x0, WebKit#104] \n"             // LowLevelInterpreter.asm:1009
    ...

Now, it can emit this instead:
    ".loc 1 996\n"        "ldp x19, x20, [x0, #0] \n"          // LowLevelInterpreter.asm:996
    ".loc 1 997\n"        "ldp x21, x22, [x0, WebKit#16] \n"         // LowLevelInterpreter.asm:997
    ...
    ".loc 1 1001\n"       "ldp d8, d9, [x0, WebKit#80] \n"           // LowLevelInterpreter.asm:1001
    ".loc 1 1002\n"       "ldp d10, d11, [x0, WebKit#96] \n"         // LowLevelInterpreter.asm:1002
    ...

Also, there was some code that kept recomputing the base address of a sequence of load/store
instructions.  For example,
    ".loc 6 902\n"        "add x13, sp, x10, lsl WebKit#3 \n"        // WebAssembly.asm:902
                          "ldr x0, [x13, WebKit#48] \n"
                          "add x13, sp, x10, lsl WebKit#3 \n"
                          "ldr x1, [x13, WebKit#56] \n"
                          "add x13, sp, x10, lsl WebKit#3 \n"
                          "ldr x2, [x13, WebKit#64] \n"
                          "add x13, sp, x10, lsl WebKit#3 \n"
                          "ldr x3, [x13, WebKit#72] \n"
    ...

For such places, we observe that the base address is the same for every load/store instruction
in the sequence, and precompute it in the LLInt asm code to help out the offline asm.  This
allows the offlineasm to now emit this more efficient code instead:
    ".loc 6 896\n"        "add x10, sp, x10, lsl WebKit#3 \n"        // WebAssembly.asm:896
    ".loc 6 898\n"        "ldp x0, x1, [x10, WebKit#48] \n"          // WebAssembly.asm:898
                          "ldp x2, x3, [x10, WebKit#64] \n"
    ...

* Source/JavaScriptCore/llint/LowLevelInterpreter.asm:
* Source/JavaScriptCore/llint/WebAssembly.asm:
* Source/JavaScriptCore/offlineasm/arm64.rb:
* Source/JavaScriptCore/offlineasm/instructions.rb:

Canonical link: https://commits.webkit.org/251799@main
Wowfunhappy referenced this pull request in Wowfunhappy/WebKit Jun 16, 2026
The legacy App Store (which links our installed WebKit2) crashed ~10s
after launch and, had it rendered, showed raw MZStore XML. Both fixed in
WKPage.cpp:

1. Off-main crash: the App Store calls WKPageLoadURL from its own
   background GCD queue ("WebView Initial Load Queue"), reaching
   WebProcessPool::pageEndUsingWebsiteDataStore where
   RELEASE_ASSERT(RunLoop::isMain()) correctly fires. New
   runLoadOnMainRunLoop helper marshals the WKPageLoadURL load family
   onto the main RunLoop when called off-main (synchronous passthrough
   when already on main, so Safari/QuickLook are unchanged), capturing
   page/userData by Ref. The work now runs on main; the assert holds
   legitimately (NOT weakened).

2. Raw XML: narrowed the backport's V0 decidePolicyForResponse bypass
   with && !frame.isMainFrame(), so the App Store's main-frame V0 policy
   client again intercepts the text/xml MZStore plist instead of WebKit
   auto-allowing it and rendering it raw. Upstream has no such bypass;
   this shrinks the deviation back toward upstream. No download
   regression (the bypass only ever fired for canShowMIMEType==true; real
   downloads are canShowMIMEType==false and already reached the client).

Verified: App Store renders the live Featured storefront, stable 7+ min;
Safari unregressed. Hack-checked clean. Follow-up debt: the remaining
subframe V0 bypass is still non-upstream (#60).

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Wowfunhappy referenced this pull request in Wowfunhappy/WebKit Jun 18, 2026
The backport installed a global NSEvent monitor (+[WKView installEventMonitorOnce]) that intercepted mouse/scroll/key events and dispatched them to WKView, on the belief that layer-tree mirror subviews intercepted hit-testing. That was a MISDIAGNOSIS: WKView hosts the WebContent layer tree as a plain CALayer sublayer of its backing layer (MinimalPageClient enterAcceleratedCompositingMode/setRemoteLayerTreeRootNode, [[m_view layer] addSublayer:]) with ZERO NSView subviews. CALayers don't participate in NSView -hitTest:, so nothing ever intercepted events. Worse, the monitor consumed events before -[NSWindow sendEvent:] (where AppKit activates the window on click), creating the very click-to-activate problem it then re-patched.

Remove the monitor and rely on AppKit's natural responder chain; WKView's own scrollWheel:/keyDown:/mouseDown:/mouseMoved: handlers receive events directly. Keep the -hitTest: override as upstream-equivalent insurance (upstream WebViewImpl::hitTest does the same redirect).

Verified live: main-view links/scroll/typing/right-click; hover (#17) fires :hover on a fresh page AND with the address bar focused; Reader scroll (#39) works via natural routing to the topmost ReaderWKView; and every secondary WKView that relied on the monitor -- uBlock popover (#14/#23), Web Inspector (#16), App Store (#72), Top Sites (#13), Preferences>Extensions (#24) -- stays interactive. No crashes. Retires the #62 event-monitor keystone and supersedes #39's monitor routing. (The DDG-logo black box, a separate #56 IOSurface-backing symptom, is root-caused but not fixed here.)

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants