crash in libsystem_kernel.dylib@0x16db6 when using Pinboard bookmarklet

RESOLVED FIXED in Firefox 48

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: cpeterson, Assigned: haik)

Tracking

(Blocks 1 bug, {crash, regression, reproducible})

Trunk
mozilla49
Unspecified
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s?, firefox45 unaffected, firefox46 unaffected, firefox47 unaffected, firefox48+ fixed, firefox49 fixed)

Details

(Whiteboard: btpp-fixnow, crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

3 years ago
This bug was filed from the Socorro interface and is 
report bp-d9a3ec18-9358-4abb-af89-2e67e2160421.
=============================================================

STR:
1. You need a pinboard.in account.
2. Copy the Pinboard "read later" bookmarklet to your bookmark toolbar: https://pinboard.in/howto/#saving
3. Click the "read later" bookmarklet.

RESULT:
The bookmarklet will pop up a small window, as expected, and then the content process will crash.
Reporter

Comment 1

3 years ago
[Tracking Requested - why for this release]:

Haik, I suspect this crash is a regression from fix for window resizing bug 1258925. I bisected this crash to the following pushlog:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a338aa9ac43ff12190109267d2210d230db9286e&tochange=a00a1a9a5a7f947c987b7e8c79182ceefa0f7f18
Blocks: 1258925
Flags: needinfo?(haftandilian)
Reporter

Comment 2

3 years ago
btw, I also noticed some strange page layout issues when switching tabs this morning. The page elements slowly moved from the side of the page to their correct positions, as if they were adapting to an expanding window size, even though the window size was already maximized. I don't know if that layout issue is related to this crash.
Component: IPC → DOM
Reporter

Comment 3

3 years ago
This crash only happens when e10s is enabled.
tracking-e10s: --- → ?
Keywords: reproducible
Whiteboard: btpp-fixnow
Assignee

Comment 4

3 years ago
The crashing thread was in PBrowserChild::SendGetTabCount(unsigned int*) which was added by my fix for 1258925. I'll take this one.
Flags: needinfo?(haftandilian)
Assignee

Updated

3 years ago
Assignee: nobody → haftandilian
Assignee

Comment 5

3 years ago
On the chrome side, when handling the GetTabCount message, we return an error from RecvGetTabCount which triggers the content process being crashed. RecvGetTabCount returns an error because TabParent::GetXULBrowserWindow returns NULL due to the docShell having a NULL mTreeOwner. I don't understand why we don't have an mTreeOwner yet.

Updated

3 years ago
Crash Signature: [@ libsystem_kernel.dylib@0x16db6] → [@ libsystem_kernel.dylib@0x16db6] [@ WaitForSingleObjectEx | WaitForSingleObject | PR_WaitCondVar | mozilla::CondVar::Wait | mozilla::ipc::MessageChannel::WaitForSyncNotify | mozilla::ipc::MessageChannel::Send | mozilla::dom::PBrowserChild::SendGetTabCo…
Assignee

Comment 6

3 years ago
Something about this test case causes the docShell's mTreeOwner field to be NULL and without the fix for 1258925, we have the same issue but it doesn't cause a crash because TabParent::RecvSetDimensions() ignores the resize and returns true. More investigation is needed to determine if that's a bug. For example, are resizes of some popups being ignored? I think it makes sense to fix the crash by taking the same approach in TabParent::RecvGetTabCount() and returning true in this case. Then looking into why the mTreeOwner member is NULL and determining if that's a existing bug.
Comment on attachment 8744200 [details] [diff] [review]
Changes RecvGetTabCount to return true when the docShell has no tree owner

Review of attachment 8744200 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/TabParent.cpp
@@ -3251,4 @@
>  
>    uint32_t tabCount;
>    nsresult rv = xulBrowserWindow->GetTabCount(&tabCount);
>    NS_ENSURE_SUCCESS(rv, false);

Could you change this one too?
Attachment #8744200 - Flags: review?(gkrizsanits) → review+
Assignee

Comment 9

3 years ago
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> Comment on attachment 8744200 [details] [diff] [review]
> Changes RecvGetTabCount to return true when the docShell has no tree owner
> 
> Review of attachment 8744200 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/TabParent.cpp
> @@ -3251,4 @@
> >  
> >    uint32_t tabCount;
> >    nsresult rv = xulBrowserWindow->GetTabCount(&tabCount);
> >    NS_ENSURE_SUCCESS(rv, false);
> 
> Could you change this one too?

Sure, but I don't have a good idea of what would cause that GetTabCount call to fail and if it's something we would want to trigger a content crash for. Ignoring it seems harmless though. I think it's more important to figure out why the docShell's mTreeOwner is NULL.

I'll update the patch to return true for GetTabCount failures and to also set the tabCount to zero when the function starts and add a comment explaining a zero tabCount means we failed to get the number of tabs for the window.
Comment on attachment 8744465 [details] [diff] [review]
Changes RecvGetTabCount to return true/tabCount=0 when GetXULBrowserWindow or GetTabCount fails

Review of attachment 8744465 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Haik Aftandilian [:haik] from comment #9)
> Sure, but I don't have a good idea of what would cause that GetTabCount call
> to fail and if it's something we would want to trigger a content crash for.

I just want to avoid confusion with it. The other option is to not check the return
value at all. The advantage of this version is that it's more future proof.

> Ignoring it seems harmless though. I think it's more important to figure out
> why the docShell's mTreeOwner is NULL.

Most of the recv functions are doing the same thing so I wouldn't worry too much about it.
If we return null we let the change happen, if there is no other reason to block (we would
only block the request if tab count > 1). What important is that if there are multiple tabs
in the docshell, then we return something bigger than 1.

> 
> I'll update the patch to return true for GetTabCount failures and to also
> set the tabCount to zero when the function starts and add a comment
> explaining a zero tabCount means we failed to get the number of tabs for the
> window.

This sounds good, thanks.

::: dom/ipc/TabParent.cpp
@@ +3246,5 @@
> +/**
> + * Returns the number of tabs in the window via the out parameter.
> + * If the number of tabs can't be determined, returns 0.
> + *
> + * @param aValue where to store the tab count

Method documentations like this usually goes to the header.
Attachment #8744465 - Flags: review?(gkrizsanits) → review+
Assignee

Updated

3 years ago
Keywords: checkin-needed
Assignee

Updated

3 years ago
Blocks: 1267367

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/df963d1bc915
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Target Milestone: mozilla48 → mozilla49
Haik, this was mentioned as a top crasher on Aurora48. Do you think the fix is safe and the Nightly data looking promising so we can uplift this to Aurora?
Flags: needinfo?(haftandilian)
Assignee

Comment 16

3 years ago
(In reply to Ritu Kothari (:ritu) from comment #15)
> Haik, this was mentioned as a top crasher on Aurora48. Do you think the fix
> is safe and the Nightly data looking promising so we can uplift this to
> Aurora?

Yes, I think it's OK to uplift this to 48. The fix is low risk.
Flags: needinfo?(haftandilian)

Updated

3 years ago
Crash Signature: [@ libsystem_kernel.dylib@0x16db6] [@ WaitForSingleObjectEx | WaitForSingleObject | PR_WaitCondVar | mozilla::CondVar::Wait | mozilla::ipc::MessageChannel::WaitForSyncNotify | mozilla::ipc::MessageChannel::Send | mozilla::dom::PBrowserChild::SendGetTabCo… → [@ libsystem_kernel.dylib@0x16db6] [@ WaitForSingleObjectEx | WaitForSingleObject | PR_WaitCondVar | mozilla::CondVar::Wait | mozilla::ipc::MessageChannel::WaitForSyncNotify | mozilla::ipc::MessageChannel::Send | mozilla::dom::PBrowserChild::SendGetTabCo…
For it to be uplifted, you should request approval for aurora.
Flags: needinfo?(haftandilian)
Assignee

Updated

3 years ago
Attachment #8744954 - Flags: approval-mozilla-aurora?
Assignee

Updated

3 years ago
Flags: needinfo?(haftandilian)
Assignee

Updated

3 years ago
Attachment #8744954 - Flags: approval-mozilla-aurora?
Assignee

Comment 18

3 years ago
Removed approval-mozilla-aurora? while I sanity test on Aurora.

Updated

3 years ago
Duplicate of this bug: 1270904
Assignee

Comment 20

3 years ago
Comment on attachment 8744954 [details] [diff] [review]
Changes RecvGetTabCount to return true/tabCount=0 when GetXULBrowserWindow or GetTabCount fails

Approval Request Comment
[Feature/regressing bug #]:1258925
[User impact if declined]:Content process crashes on some pages that trigger popups hence bad user experience.
[Describe test coverage new/current, TreeHerder]:In mozilla-central since 2016-04-26.
Nightly try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=36e9dc4985ae
Inbound try:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=df963d1bc915
An earlier version of the fix, but results still relevant:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e154f1c3c0e
[Risks and why]:Low risk. The changes are small and only affect the area of code added for 1258925.
[String/UUID change made/needed]:N/A
Attachment #8744954 - Flags: approval-mozilla-aurora?
Assignee

Comment 21

3 years ago
Patch applies cleanly to Aurora. Did manual testing confirming the problem exists in Aurora, but is not reproducible in debug or non-debug with the patch applied.
Comment on attachment 8744954 [details] [diff] [review]
Changes RecvGetTabCount to return true/tabCount=0 when GetXULBrowserWindow or GetTabCount fails

Crash fix, manual testing on aurora, let's uplift it!
Attachment #8744954 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking since this is a recent regression.
Version: unspecified → Trunk
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.