Closed Bug 1378036 Opened 2 years ago Closed 2 years ago

JavaScript error: resource:///modules/ContentCrashHandlers.jsm, line 135: TypeError: WeakMap key must be an object, got undefined

Categories

(Toolkit :: Crash Reporting, enhancement)

56 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: whimboo, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

As noticed in bug 1352671 comment #44 there is a Javascript error when handling a "oop-frameloader-crashed" situation:

> 19:05:03     INFO - GECKO(1144) | JavaScript error:
> resource:///modules/ContentCrashHandlers.jsm, line 135: TypeError: WeakMap
> key must be an object, got undefined

Source:

https://dxr.mozilla.org/mozilla-central/rev/a3b192dc8344679ce208af42b6246c3c0d42cab3/browser/modules/ContentCrashHandlers.jsm#127-137

Mike, given that you added this code a while ago, maybe you could have a look at it? We would appreciate given that it seems to shadow a crash which causes a high frequent intermittent failure. Thanks.
Flags: needinfo?(mconley)
Component: General → Crash Reporting
Product: Firefox → Toolkit
(In reply to Henrik Skupin (:whimboo) from comment #0)
> Mike, given that you added this code a while ago, maybe you could have a
> look at it? We would appreciate given that it seems to shadow a crash which
> causes a high frequent intermittent failure. Thanks.

Hm... so in this case, we have a browser without a permanent key crashing - which means a browser that's somehow not got the <xul:browser> binding attached to it (yet). Perhaps it's crashing before the binding is fully attached.

What do we want to happen here? Do we want this function to return early, like if there's no browser passed as the subject? Will that allow the crash handler to kill Firefox in this case?
Flags: needinfo?(mconley) → needinfo?(hskupin)
I would gently forward this question to Ted. But I think we should still shutdown the browser if the environment variable has been set.
Flags: needinfo?(hskupin) → needinfo?(ted)
Blocks: 1378624
I really don't understand this code very well, but it does seem like we should either gracefully handle this, or track down the root cause and fix it so it doesn't happen. If we really got serious I guess we could add telemetry to see if this is happening in the wild.
Flags: needinfo?(ted)
Hey felipe, I'm eager to hear what you think of this. We're basically trading the ability to track dump IDs for bindingless <browser> elements, with the ability to track browsers across frameloader swaps.

I suppose an alternative would be to store the permanentKey in the WeakMap, and if it doesn't exist, fallback to the <browser> instead. Should we go that route?
Flags: needinfo?(felipc)
Hey Kevin,

Over IRC, felipe suggested I redirect comment 5 at you, particularly because of your amazing work with lazy tabs. Do you happen to know if we have some kind of related identifier attached to <browser>'s that survive even when the binding hasn't yet been applied, or the tab has been dragged to a new window?
Flags: needinfo?(felipc) → needinfo?(kevinhowjones)
(In reply to Mike Conley (:mconley) - Digging out of needinfo / review backlog. from comment #6)
> Hey Kevin,
> 
> Over IRC, felipe suggested I redirect comment 5 at you, particularly because
> of your amazing work with lazy tabs. Do you happen to know if we have some
> kind of related identifier attached to <browser>'s that survive even when
> the binding hasn't yet been applied, or the tab has been dragged to a new
> window?

permanentKey is attached immediately after the browser is created:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2046

This should persist whether the binding is ever applied or not.

I am not familiar with any code that removes browser.permanentKey except on tab removal.

ie, a tab-browser should have permanentKey regardless of whether the browser ever gets bound.
Flags: needinfo?(kevinhowjones)
Could it be that the browser is losing the permanent key due to some remote/non-remote transition? IIRC there's some situation where the browser is unbound and then gets bound again.. Maybe that path is not carrying the permanent key?

Or maybe it's caused by this? https://dxr.mozilla.org/mozilla-central/rev/20f32734df750bddada9d1edca665c2ea53946f0/browser/base/content/tabbrowser.xml#3363
I don't understand why we would want to swap the browsers but not the permanent keys, as in this case.
(In reply to :Felipe Gomes (needinfo me!) from comment #8)
> Could it be that the browser is losing the permanent key due to some
> remote/non-remote transition? IIRC there's some situation where the browser
> is unbound and then gets bound again.. Maybe that path is not carrying the
> permanent key?

I don't think that would matter.  permanentKey is independent of whether the browser is bound or not.  The browser should still hold permanentKey even if it gets unbound.  It seems to me there has to be some action that explicitly is removing it.

> Or maybe it's caused by this?
> https://dxr.mozilla.org/mozilla-central/rev/
> 20f32734df750bddada9d1edca665c2ea53946f0/browser/base/content/tabbrowser.
> xml#3363
> I don't understand why we would want to swap the browsers but not the
> permanent keys, as in this case.

I don't think that of itself would do it, because the error seems to be indicating the browser does not have a permanentKey.  Even if keys don't get swapped, each browser would still keep the key it has.

Whether or not the keys not getting swapped properly would trigger the key getting mistakenly removed down the line, eg, a tab gets removed which linked one of those browsers, I don't know.  But it doesn't seem like that should be a possibility to me; AFAIK, there is no reverse referencing of the key back to the browser.
The other possibility is that in this case, the browser that's crashing isn't actually a tabbrowser browser, so it's never received a permanentKey.

Considering all of this, I think I'm starting to prefer the plan of using permanentKey first, and then falling back to the <browser> element itself as the WeakMap keys.
So.. it's very likely what you said, that this is the case for a non-tabbrowser browser..

So your patch looks reasonable, as in: let's use browsers as the keys instead of the permanentKey. But it's a bit unfortunate that we'd lose track of the crash report if it's dragged out of the window. Could we add something to swapBrowsers to fix that?

But that brings the question: this code is from the Tab crash handler.. So, should it even care about non-tabbrowser browsers?  Perhaps the best is to just make sure it doesn't throw the error originally reported?
Assignee: nobody → mconley
Comment on attachment 8884082 [details]
Bug 1378036 - TabCrashHandler should key browsers to dumpIDs on permanentKeys, and fallback to the browser itself if no permanentKey exists.

https://reviewboard.mozilla.org/r/155018/#review163834

::: browser/modules/ContentCrashHandlers.jsm:49
(Diff revision 2)
> + * permanentKey. If, however, the browser has had its permanentKey removed,
> + * it falls back to keying on the <xul:browser> element itself.

update the comment saying it's more about a permanentKey never existing, instead of being removed.. (which I believe it's our current theory)

::: browser/modules/ContentCrashHandlers.jsm:52
(Diff revision 2)
> + *
> + * Under the hood, BrowserWeakMap keys the map off of the <xul:browser>
> + * permanentKey. If, however, the browser has had its permanentKey removed,
> + * it falls back to keying on the <xul:browser> element itself.
> + */
> +class BrowserWeakMap extends WeakMap {

classes... fancy!
Attachment #8884082 - Flags: review?(felipc) → review+
Comment on attachment 8887246 [details]
Bug 1378036 - Factor out a utility method for about:tabcrashed tests.

https://reviewboard.mozilla.org/r/158046/#review163838
Attachment #8887246 - Flags: review?(felipc) → review+
Comment on attachment 8887247 [details]
Bug 1378036 - Add a regression test for tabs crashing without a permanentKey.

https://reviewboard.mozilla.org/r/158048/#review163840
Attachment #8887247 - Flags: review?(felipc) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7ca046bede5
TabCrashHandler should key browsers to dumpIDs on permanentKeys, and fallback to the browser itself if no permanentKey exists. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/7871c16d1652
Factor out a utility method for about:tabcrashed tests. r=Felipe
https://hg.mozilla.org/integration/autoland/rev/653720e594d1
Add a regression test for tabs crashing without a permanentKey. r=Felipe
Blocks: 1325530
Duplicate of this bug: 1346418
Blocks: 1497073
You need to log in before you can comment on or make changes to this bug.