e10s: Javascript errors should be proxied to chrome

RESOLVED FIXED

Status

()

Core
IPC
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

unspecified
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

(Whiteboard: [fennec-checkin-postb1])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

8 years ago
The silent failures I keep getting are starting to make me very sad.  Digging through debug console output is not a fun process.
(Assignee)

Updated

8 years ago
Blocks: 516752
(Assignee)

Comment 1

8 years ago
Created attachment 429369 [details] [diff] [review]
WIP basic remoting of nsConsoleService::LogMessage

This just does a straight proxy of LogMessage, so everything shows up in the parent as a generic console message.  This is less than ideal, but gets the job done for the time being.
Assignee: nobody → josh
(Assignee)

Updated

8 years ago
Depends on: 528250
(Assignee)

Comment 2

8 years ago
Note: patch depends on the build tier unification in bug 528250.
(Assignee)

Comment 3

8 years ago
Created attachment 429383 [details] [diff] [review]
WIP proper remoting of script errors

This is better, and actually gets the job done.  I think I can one up it, though.
Attachment #429369 - Attachment is obsolete: true
(Assignee)

Comment 4

8 years ago
Created attachment 429386 [details] [diff] [review]
Delegate message remoting to each message type

Unlike previous patches, messages now remote themselves.  The system appears to work flawlessly.
Attachment #429383 - Attachment is obsolete: true
Attachment #429386 - Flags: review?(benjamin)
(Assignee)

Comment 5

8 years ago
Hmm, there's something fishy happening on shutdown where the child process aborts when trying to send a script error.  I'm looking into that; I would still appreciate a review of the current patch because I believe the method is sound.

Updated

8 years ago
Attachment #429386 - Flags: review?(benjamin) → review?(bzbarsky)
Hmm.  So a few questions:

1)  Wouldn't it make more sense to do the sending in the console service instead?
2)  LogMessage can happen on an arbitrary thread.  Can the SendRemoteMessage
    stuff run on an arbitrary thread?

Comment 7

8 years ago
No, IPDL methods have to be main-thread only.
Another option is to add a console listener in the child process which sends stuff over to the parent process...
Just in terms of where to put this code, that is.  The main drawback is it would have to know about the kinds of console messages that can happen...
(Assignee)

Updated

8 years ago
Blocks: 556846
(Assignee)

Comment 10

8 years ago
I've got a new version which dispatches an event to the main thread to trigger the IPDL message.  In my mind, forcing message types to be aware of how to remote themselves is cleaner than an all-knowing listener or service.  It also reduces  boilerplate significantly for things like nsScriptError (ie. rv = message->GetX(&x); NS_ENSURE_SUCCESS(rv, rv)).  However, if you'd prefer the omnipotent version, I can whip that up as well.
(Assignee)

Comment 11

8 years ago
Created attachment 442909 [details] [diff] [review]
Patch

This is effectively the same patch as previously, but now doesn't break when messages are logged from arbitrary threads.
Attachment #429386 - Attachment is obsolete: true
Attachment #442909 - Flags: review?(bzbarsky)
Attachment #429386 - Flags: review?(bzbarsky)
+++ b/dom/ipc/Makefile.in
 		-I$(topsrcdir)/chrome/src \
+		-I$(srcdir)/../../xpcom/base \

I'd say

+		-I$(topsrcdir)/xpcom/base \

makes more sense.
So with this patch, console listeners in the content process see no messages.  Is that desired?

If not, it seems cleaner to allow listeners but have a listener that calls SendRemoteMessage.  Listeners _are_ notified on the main thread (the console service handles this), and that should make things Just Work.  Might even be less boilerplate...

Updated

7 years ago
tracking-fennec: --- → 2.0+
(Assignee)

Comment 14

7 years ago
Comment on attachment 442909 [details] [diff] [review]
Patch

New patch coming up.
Attachment #442909 - Attachment is obsolete: true
Attachment #442909 - Flags: review?(bzbarsky)
(Assignee)

Comment 15

7 years ago
Created attachment 477531 [details] [diff] [review]
Remote console messages from content to chrome via a listener.
(Assignee)

Updated

7 years ago
Attachment #477531 - Flags: review?(bzbarsky)
Comment on attachment 477531 [details] [diff] [review]
Remote console messages from content to chrome via a listener.

>+++ b/dom/ipc/ContentChild.cpp
>+        nsAutoPtr<char> category;
>+        char *tmp;

No, this is totally broken.  This will deallocate with delete[] memory that was allocated with some other allocator.

The right way to do this is:

  nsXPIDLCString category;
  .....
  rv = scriptError->GetCategory(getter_Copies(category));


r=me with that fixed
Attachment #477531 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

7 years ago
Attachment #477531 - Attachment is obsolete: true
(Assignee)

Comment 17

7 years ago
Created attachment 478788 [details] [diff] [review]
Remote console messages from content to chrome via a listener.

Incorrect string usage addressed.
(Assignee)

Updated

7 years ago
Whiteboard: [fennec-checkin-postb1]
tracking-fennec: 2.0+ → 2.0b2+
pushed http://hg.mozilla.org/mozilla-central/rev/5f0370936fc9
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.