549682 - Port the message-manager API to mozilla-central

Status: RESOLVED FIXED

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

Description

[Benjamin Smedberg]

As soon as the message-manager API is reasonably stable-seeming, we should port it to mozilla-central so that code can start transitioning to it without having e10s ifdefs.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This is a dup of bug 534254, but I'll mark that to be a dup of this.

And yes, I plan to do this once the API is reasonable stable.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip, misses still all the JS handling etc, so basically perhaps 3/5 done.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: test browser

This has basically all the same tests as for e10s mm, but without CPOW
(which is something in-process-mm won't support, since CPOWs will be removed
from e10s too)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

This should behave pretty closely like e10s-mm.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

When merging e10s to m-c, some of the code duplication can be removed.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Doug, would be great if this could be tested with Fennec.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Seem like the patch may not apply quite clean with trunk, at least not without
some --fuzz

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

updated patch coming..

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

One thing I may need to change, and which would work differently than in
E10s is script loading.
In E10s it happens asynchronously, but I think in non-E10s it should
happen sync so that scripts are ready when the page is being loaded.

The change would be trivial; just don't use nsAsyncScriptLoader,
but call LoadRemoteScript in LoadScript.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

And to clarify, the patch is for m-c, not for e10s branch.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Hmm, tryserver doesn't seem to like the patch.
I need to look at the failures.
But I think it is still ok to review the patch.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #13)
> Hmm, tryserver doesn't seem to like the patch.
...which is not very surprising. Changing how events are propagated is
rather regression risky.

Comment 15

[Benjamin Stover (:stechz)]

Tried this patch out with e10s Fennec, and this gets us a long way towards the goal of working with remote and non-remote browsers.  Great work!

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Thanks for testing. I'll try to fix the tryserver failures today/tomorrow and
I'll probably change the script loading to happen synchronously.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

The previous patch was missing the case where frameloaders are swapped.
I think that is a case which isn't handled at all in E10s atm.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Locally chrome tests pass now, but I posted the patch anyway to tryserver.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Uh, tryserver doesn't like the patch. Investigating.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Yet another patch.  Crossing fingers, maybe tryserver likes this.
At least testing this locally looks promising.
The previous patch was actually unlinking too much in some cases.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

The latest patch seems to have passed tests on tryserver, though
some random looking orange in some builds.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 444478 [details] [diff] [review]
patch

Bah, I found still something to fix. And chrome tests are coming too.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v7

JSPrincipal handling is extremely ugly, and not documented at all.
This patch doesn't leak principals anymore. I'll fix E10s in bug 565175
(if the problem is also in E10s).

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 444767 [details] [diff] [review]
v7

Tryserver liked this better :)

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 444767 [details] [diff] [review]
v7

Looks good.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Landed http://hg.mozilla.org/mozilla-central/rev/b62ac40565f2
But let's see if that works before marking this fixed.

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

In nsContentUtils.cpp:

    7.38 -  if (!aDoc->GetWindow()->GetChromeEventHandler())
    7.39 +
    7.40 +  nsPIDOMEventTarget* piTarget = aDoc->GetWindow()->GetChromeEventHandler();
    7.41 +  if (!target)
    7.42      return NS_ERROR_INVALID_ARG;

Shouldn't this check for piTarget instead?

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Indeed. Thanks!

I'll fix in a followup hg push, if the patch keeps tbox green.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Backed out. There is some crash.
http://hg.mozilla.org/mozilla-central/rev/5127e4bfa127

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Attachment: keep MM alive while calling loadFrameScript

This fixes also the piTarget thing.

I'll push this. At least locally I can't reproduce the problem tbox showed.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Only known oranges. Marking fixed.
Please open new bugs for issues and feature requests.

Comment 32

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

smaug, these changes make for a tricky merge of m-c to e10s.  Do you mind doing it?

Comment 33

[Robert Sayre]

I was looking at this checkin while merging TM, and it looks there are a lot of JSAPI calls with unchecked return values. What's up with that?

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

What *a lot calls* with unchecked return values?
It is ok, IMO,  if JS_DefineProperty fails in this case, and in other cases the
return value is checked.
There is also no need to check the return value of JS_EvaluateUCScriptForPrincipals, as far as I see.


And about merging, yes, I guess I need to do that.
Before starting to write the patch I was arguing that it should be written
for e10s, not m-c, but others didn't agree.

Comment 35

[Brendan Eich [:brendan]]

(In reply to comment #34)
> What *a lot calls* with unchecked return values?
> It is ok, IMO,  if JS_DefineProperty fails in this case, and in other cases the
> return value is checked.
> There is also no need to check the return value of
> JS_EvaluateUCScriptForPrincipals, as far as I see.

It's not ok to fail to report an uncaught exception. Is this API call guaranteed to be happening when cx has no active frames on it (!JS_IsActive(cx))?

If so, ok; the engine will report uncaught exceptions as this API unwinds (there is a JS context option, JSOPTION_DONT_REPORT_UNCAUGHT, that you can set to avoid this default behavior).

If not, then you are adding hard-to-find bugs by suppressing failure (exception, error) instead of propagating it.

/be

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

MDC documentation doesn't say anything about handling uncaught exceptions.
It says just "On success, JS_DefineProperty returns JS_TRUE. If the property already exists or cannot be created, JS_DefineProperty returns JS_FALSE."

And for JS_EvaluateUCScriptForPrincipals, the code comes from e10s content
process, where there isn't any place to report any exceptions (currently).
And MDC says just "If the script compiles and executes successfully, *rval receives the value from the last-executed expression statement processed in the script, and JS_EvaluateScriptForPrincipals or JS_EvaluateUCScriptForPrincipals returns JS_TRUE. Otherwise it returns JS_FALSE, and the value left in *rval is undefined. "
Nothing about exceptions. Based on that how could API user know that
one should handle the return value?


(jsapi.h is even more scarce than MDC)

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

And how does the exception propagate from JS_EvaluateUCScriptForPrincipals?
Does it propagate to caller? (Which should be JS). That would be
pretty reasonable behavior for loadFrameScript().

Comment 38

[Brendan Eich [:brendan]]

jsapi.h docs are on MDC, not in the .h file itself; see specifically

https://developer.mozilla.org/En/SpiderMonkey/JSAPI_User_Guide#Errors_and_exceptions

See also

https://developer.mozilla.org/En/SpiderMonkey/JSAPI_Phrasebook

jst and mrbkap know the drill; so do others.

/be

Comment 39

[Robert Sayre]

This is what I spotted:

179       JS_Stringify(ctx, &v, nsnull, JSVAL_NULL, JSONCreator, &aJSON);
(unchecked)

213       for (PRUint32 i = 0; i < len; ++i) {
214         jsval ret = JSVAL_VOID;
215         nsAutoGCRoot root(&ret, &rv);
216         NS_ENSURE_SUCCESS(rv, rv);
217         JSONParser* parser = JS_BeginJSONParse(ctx, &ret);
(needs null check)

218         JSBool ok = JS_ConsumeJSONText(ctx, parser, (jschar*)retval[i].get(),
219                                        (uint32)retval[i].Length());
220         ok = JS_FinishJSONParse(ctx, parser, JSVAL_NULL) && ok;
221         if (ok) {
222           dest[i] = ret;
223         }
(loop continues after failure)

224       }


403         JS_CallFunctionValue(mContext, thisObject,
404                              funval, 1, argv, &rval);
(unchecked)

408               JS_Stringify(mContext, &rval, nsnull, JSVAL_NULL,
409                            JSONCreator, &json)) {
(unchecked)

Comment 40

[Brian Crowder]

+    nsCOMPtr<nsIContentFrameMessageManager> mm = mChildManagers[i];

This needs to become

+    nsCOMPtr<nsIContentFrameMessageManager> mm = do_QueryInterface(mChildManagers[i]);

probably due to a recent change in e10s?  ....  want a new bug for this?

Comment 41

[Nickolay_Ponomarev]

dev-doc-needed: https://wiki.mozilla.org/Content_Process_Event_Handlers

Comment 42

[Eric Shepherd [:sheppy]]

I have written this up. Due to its deeply technical nature, I would appreciate it if someone would give it a thorough reading-over to ensure there's nothing painfully misleading here.

Added this page:

https://developer.mozilla.org/en/Content_process_event_handling

Also added the following reference pages:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIFrameLoader
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIChromeFrameMessageManager
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIFrameMessageListener
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIContentFrameMessageManager
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIInProcessContentFrameMessageManager
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsISyncMessageSender
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIFrameLoaderOwner