Fire an event that allows mozbrowser embedders to deduce that mozbrowser content has crashed/died

RESOLVED FIXED in mozilla17

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 attachment)

I'm in favor of not introducing OS processes into the "web abstractions", so if we can repurpose existing events to this end, I would be happier than with a new one like "oncrash".

Any ideas?

We need this work to allow gaia to detect when apps are lowmemkilled/die/crash.  Currently we're just left with a blank iframe, and AFAIK gaia doesn't know anything has happened.
Just throwing an idea here: can't we change the location to "about:crashed" which will fire a "locationchange" event so the embedder will be able to check the new url?
We can't navigate mozapp frames.
(Assignee)

Comment 3

5 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> We can't navigate mozapp frames.

Maybe you mean something different by "we" than Mounir does.

If the process hadn't just crashed, we can navigate the mozapp frame wherever we want.  There are some same-origin restrictions, but we could (and probably would need to) relax those for about: pages.

Since the process has crashed, we can't navigate the frame anywhere without either starting up a new content process (probably a bad idea) or switching to an in-process browser frame (possibly difficult).

We could fire a fake locationchange to "about:crashed", so long as pages (apps and browsers) couldn't navigate themselves to that location...
Can we not just reuse something like onunload?  I feel like we're trying to get too cute here.
(Assignee)

Comment 5

5 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Can we not just reuse something like onunload?  I feel like we're trying to
> get too cute here.

If I have an in-process iframe, iframe.onload fires when a new page loads into the iframe.  It seems to me that using iframe.onunload for "this iframe crashed" is not at all the same sort of thing.
(In reply to Justin Lebar [:jlebar] from comment #3)
> We could fire a fake locationchange to "about:crashed", so long as pages
> (apps and browsers) couldn't navigate themselves to that location...

AFAIK, pages can't navigate themselves to about: pages.
So, I'm not sure why we don't go for that solution?
David, Ben, suppose a "window" in the window manager crashes, or a "tab" in the browser crashes.  What would be the most convenient way to notify you?

(I tried to CC Dale but I can't seem to find his account in bz.)
djf and I discussed this for a bit on IRC.  Another issue with crashed content is that after it crashes, the pixels in the iframe are basically "undefined".  (Should normally be blank, but I'm not 100% sure.  Probably whatever the iframe background style is.)

So I propose something along the lines of what Mounir suggested
 - let the mozbrowser embedder set a "fallback URI", or "error URI" (whatever, name TBD)
 - when content crashes, gecko is notified asap (not instantly, but soon)
 - gecko navigates the iframe (in-process) to the fallback URI.  The "app" loses its old security principal and gets a totally untrusted one.
 - the mozbrowser embedder is notified through the normal locationchange event

There are two use cases I have in mind.  The first is, the embedder sets the fallback URI to something like "about:blank?nonce=[randomnumber]" and stores the random number on the iframe element.  When there's a location change to "about:blank?nonce=[randomnumber]", the embedder determines there was a crash and kills the window.

Second case is, embedder sets the fallback URI to something like "https://embedder/reportcrash.html?uri=[content URI]".  When there's a location change, the embedder does nothing.  The user has the option of reporting the crash, or just closing the tab/app.

wdyt?
I dont think there is any problem with either locationchange or unload in the embedder, unload seems to make the most sense to me, but locationchange would be fine

One thing I dont understand if we dont start a new content process, we will want to be changing the page contents to some "page has crashed" page, and the user will expect to be able to navigate onwards, although we could just replace with a new tab in the browser, (I would expect mozapp to just close)
A mozbrowsercrashed/mozappcrashed event on the iframe would be the obvious answer but like you say, that introduces operating system concepts into the web layer which is probably wrong.

I actually quite like the idea of a mozbrowserlocation changed event to about:crashed because it doesn't break the abstraction.

Alternatively you could fire an alert event, informing the user that the page has crashed but that's maybe a little clunky.
"about:crashed" or $GECKO_URI_FOO is not a path to a standardizable API.  It's just more gecko magic to build into the clients.
But to be perfectly honest, I'm still happy with firing some event that means "crashed" but doesn't adopt the OS-process terminology.
(Assignee)

Comment 13

5 years ago
From an implementation point of view, replacing the OOP iframe with an in-process iframe is tricky.

My preference would be to simplify the approach in comment 8 to the following:

 - When the content process crashes, we fire a locationchange "about:crashed" (or whatever) event and blank the iframe's contents.
 - The iframe is now in a "crashed" state and rejects or ignores all further attempts to interact with it.
 - The embedder now can handle things as it pleases.  It can show a custom reportcrash page in a new non-remote iframe, it can give the user the option of trying to recover by creating a new remote iframe, it can close the app...whatever.

This is simpler to implement and is more flexible than the approach in comment 8.  (Not all implementations will want to replace the remote iframe with an in-process one, for example.)
(Assignee)

Comment 14

5 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> "about:crashed" or $GECKO_URI_FOO is not a path to a standardizable API. 
> It's just more gecko magic to build into the clients.

If you don't want "about:crashed", then let's fire a new event.  I think asking the embedder to specify a fallback URL would be a bad API.  For example, what do we do if the embedder doesn't specify this URL?  And again, actually loading this URL in process but inside the previously-oop iframe is complicated.
onfail

Objections?
(Assignee)

Comment 16

5 years ago
It would be "mozbrowserfail", like all the rest of the mozbrowser events, but "fail" is OK with me.
mozbrowserfail

Objections?
If we really want to use that event for crashes and only crashes, why not simply naming it 'mozbrowsercrash'?
We don't want to use it for crashes and only crashes.
No objections.

http://people.mozilla.com/~bfrancis/mozbrowserfail.png
(Assignee)

Comment 21

5 years ago
Not to re-open this can of worms, but I think we should combine this with bug 768842 (notify when we would otherwise show Gecko error pages).
(Assignee)

Comment 22

5 years ago
Marking as depends on bug 768842.  That bug adds a 'mozbrowsererror' event.  That event's detail has the form

 {type: TYPE, ... other stuff if applicable}.

For example, we could (but don't currently) send {type: neterror, url: http://does.not.exist}.

So I propose that for a crash, we send mozbrowsererror {type: fatal}.
Depends on: 768842
(Assignee)

Updated

5 years ago
Blocks: 768832

Updated

5 years ago
blocking-basecamp: --- → +
(Assignee)

Comment 23

5 years ago
Created attachment 648085 [details] [diff] [review]
Patch, v1
Attachment #648085 - Flags: review?(jones.chris.g)
Attachment #648085 - Flags: review?(21)
Attachment #648085 - Flags: review?(21) → review+
Comment on attachment 648085 [details] [diff] [review]
Patch, v1

I really like it when patches are as simple as you hope they'd be.
Attachment #648085 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 25

5 years ago
FYI, the way I tested this was to call ContentChild::QuickExit() on a timer 10 seconds after ContentChild::InitXPCOM.  But I was not determined enough to write an automated test.
(Assignee)

Comment 26

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/719b1d10b554
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 27

5 years ago
(In reply to Chris Jones [:cjones] [:warhammer] from comment #24)
> I really like it when patches are as simple as you hope they'd be.

The hardest bug I had here turned out to be a merge conflict.  :)
(Assignee)

Comment 28

5 years ago
https://github.com/mozilla-b2g/gaia/issues/3125
https://hg.mozilla.org/mozilla-central/rev/719b1d10b554
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.