1946461 - Crash in [@ mozilla::PresShell::GetRootFrame]

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

161 crash reports from 92 installs of Firefox 135.0

The reports for x64 ended with corrupted frames.

Emilio, is this a signature change or new?

Crash report: https://crash-stats.mozilla.org/report/index/57c40c3f-afdb-49c6-8861-715050250206

Reason:

EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames:

0  xul.dll  mozilla::UniquePtr<nsCSSFrameConstructor, mozilla::DefaultDelete<nsCSSFrameCo...  mfbt/UniquePtr.h:287
0  xul.dll  mozilla::UniquePtr<nsCSSFrameConstructor, mozilla::DefaultDelete<nsCSSFrameCo...  mfbt/UniquePtr.h:282
0  xul.dll  mozilla::PresShell::GetRootFrame() const  layout/base/PresShell.h:431
0  xul.dll  mozilla::dom::Event::GetOffsetCoords(nsPresContext*, mozilla::WidgetEvent*, m...  dom/events/Event.cpp:720
1  xul.dll  mozilla::dom::MouseEvent::OffsetPoint() const  dom/events/MouseEvent.cpp:404
2  xul.dll  mozilla::dom::MouseEvent::OffsetY() const  dom/events/MouseEvent.h:100
2  xul.dll  mozilla::dom::MouseEvent_Binding::get_offsetY(JSContext*, JS::Handle<JSObject...  dom/bindings/MouseEventBinding.cpp:621
3  xul.dll  mozilla::dom::binding_detail::GenericGetter(JSContext*, unsigned int, JS::Val...  dom/bindings/BindingUtils.cpp:3172
3  xul.dll  CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::...  js/src/vm/Interpreter.cpp:532
3  xul.dll  js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstru...  js/src/vm/Interpreter.cpp:628

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Masayuki has looked at this recently for bug 1680669.

Pretty sure the code in here is wrong. I thought that GetPrimaryFrameOfEventTarget would check aPresContext and guarantee that that frame is from the same pres context, but that is not necessary. So if you move the event target around it might still have a frame but the original pres context could already be torn down.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I try to reproduce this bug with like this kind of testcase, but I've not succeeded that yet.

    const div = document.getElementById("target");
    const iframe = document.querySelector("iframe");
    let clickEvent;
    div.addEventListener("click", event => clickEvent = event, {once: true});
    await test_driver.click(div);
    iframe.contentDocument.body.appendChild(div);
    iframe.hidden = true;
    assert_equals(
      {
        offsetX: clickEvent.offsetX,
        offsetY: clickEvent.offsetY,
      }.toString(),
      {
        offsetX: 0,
        offsetY: 0,
      }.toString(),
    );

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, I got it:

    const iframe = document.querySelector("iframe");
    const div = iframe.contentDocument.getElementById("target");
    let clickEvent;
    div.addEventListener("click", event => clickEvent = event, {once: true});
    await test_driver.click(div);
    document.body.appendChild(div);
    iframe.hidden = true;
    await new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(resolve)));
    assert_equals(
      {
        offsetX: clickEvent.offsetX,
        offsetY: clickEvent.offsetY,
      }.toString(),
      {
        offsetX: 0,
        offsetY: 0,
      }.toString(),
    );

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Odd, changing it to a crash test causes not reproducing the crash...

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, it seems that we skip crash tests using test driver.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1946461 - Make `Event::GetPrimaryFrameOfEventTarget` return `nullptr` if target is moved outside the original `nsPresContext` r=smaug!,emilio!

Chrome returns some computed value for offsetX and offsetY even in this
hacky case, but we should return {0, 0} for now to keep the traditional
behavior [1] and avoid the crash because Chrome's behavior is also tricky
because they cache offset values at first access. Let's change the behavior
in a separated bug.

1. https://searchfox.org/mozilla-central/rev/80343eb85f0bda693730a394496ce66e48eae561/dom/events/Event.cpp#683,702-704

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c9b281a867f3
Make `Event::GetPrimaryFrameOfEventTarget` return `nullptr` if target is moved outside the original `nsPresContext` r=smaug,emilio

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/50589 for changes under testing/web-platform/tests

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/c9b281a867f3

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1946461 - Make `Event::GetPrimaryFrameOfEventTarget` return `nullptr` if target is moved outside the original `nsPresContext` r=smaug!,emilio!

Chrome returns some computed value for offsetX and offsetY even in this
hacky case, but we should return {0, 0} for now to keep the traditional
behavior [1] and avoid the crash because Chrome's behavior is also tricky
because they cache offset values at first access. Let's change the behavior
in a separated bug.

1. https://searchfox.org/mozilla-central/rev/80343eb85f0bda693730a394496ce66e48eae561/dom/events/Event.cpp#683,702-704

Original Revision: https://phabricator.services.mozilla.com/D237172

Comment 12

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: May meet a crash in some web apps
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Run the automated test under mozilla/
• Risk associated with taking this patch: Low
• Explanation of risk level: Making it stop computing offset if the event target is moved to different document, and in the crash cases, this is exactlly same behavior as the older version
• String changes made/needed: nothing
• Is Android affected?: yes

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1946461 - Make `Event::GetPrimaryFrameOfEventTarget` return `nullptr` if target is moved outside the original `nsPresContext` r=smaug!,emilio!

Chrome returns some computed value for offsetX and offsetY even in this
hacky case, but we should return {0, 0} for now to keep the traditional
behavior [1] and avoid the crash because Chrome's behavior is also tricky
because they cache offset values at first access. Let's change the behavior
in a separated bug.

1. https://searchfox.org/mozilla-central/rev/80343eb85f0bda693730a394496ce66e48eae561/dom/events/Event.cpp#683,702-704

Original Revision: https://phabricator.services.mozilla.com/D237172

Comment 14

[Phabricator Automation]

release Uplift Approval Request

• User impact if declined: May meet the crash in some web apps
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Run the automated test under mozilla/
• Risk associated with taking this patch: Low
• Explanation of risk level: See the beta uplift request for the details, this takes back the legacy behavior in the crash cases
• String changes made/needed: none
• Is Android affected?: yes

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/ecd381744cf2

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/395f77753cf6