Closed Bug 1266484 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

Unspecified
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
e10s ? ---
firefox45 --- unaffected
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 + fixed
firefox49 --- fixed

People

(Reporter: cpeterson, Assigned: haik)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, reproducible, Whiteboard: btpp-fixnow)

Crash Data

Attachments

(1 file, 2 obsolete files)

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.
[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)
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
This crash only happens when e10s is enabled.
tracking-e10s: --- → ?
Keywords: reproducible
Whiteboard: btpp-fixnow
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: nobody → haftandilian
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.
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…
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+
(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+
Keywords: checkin-needed
Blocks: 1267367
https://hg.mozilla.org/mozilla-central/rev/df963d1bc915
Status: NEW → RESOLVED
Closed: 8 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)
(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)
Crash Signature: mozilla::dom::PBrowserChild::SendGetTabCount] [@ WaitForSingleObjectEx | PR_WaitCondVar | mozilla::CondVar::Wait | mozilla::ipc::MessageChannel::WaitForSyncNotify | mozilla::ipc::MessageChannel::Send | mozilla::dom::PBrowserChild::SendGetTabCount] → mozilla::dom::PBrowserChild::SendGetTabCount] [@ WaitForSingleObjectEx | PR_WaitCondVar | mozilla::CondVar::Wait | mozilla::ipc::MessageChannel::WaitForSyncNotify | mozilla::ipc::MessageChannel::Send | mozilla::dom::PBrowserChild::SendGetTabCount] [@ I…
For it to be uplifted, you should request approval for aurora.
Flags: needinfo?(haftandilian)
Attachment #8744954 - Flags: approval-mozilla-aurora?
Flags: needinfo?(haftandilian)
Attachment #8744954 - Flags: approval-mozilla-aurora?
Removed approval-mozilla-aurora? while I sanity test on Aurora.
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?
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
You need to log in before you can comment on or make changes to this bug.