The default bug view has changed. See this FAQ.

Port the message-manager API to mozilla-central




7 years ago
6 years ago


(Reporter: bsmedberg, Assigned: smaug)


(Depends on: 2 bugs, {dev-doc-complete})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(4 attachments, 4 obsolete attachments)



7 years ago
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.
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.
Duplicate of this bug: 534254
Created attachment 443742 [details] [diff] [review]
wip, misses still all the JS handling etc, so basically perhaps 3/5 done.
Created attachment 443867 [details] [diff] [review]
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)
Created attachment 443869 [details] [diff] [review]

This should behave pretty closely like e10s-mm.
Attachment #443869 - Flags: review?(jst)
When merging e10s to m-c, some of the code duplication can be removed.
Doug, would be great if this could be tested with Fennec.
Seem like the patch may not apply quite clean with trunk, at least not without
some --fuzz
updated patch coming..
Created attachment 443872 [details] [diff] [review]
Attachment #443869 - Attachment is obsolete: true
Attachment #443872 - Flags: review?(jst)
Attachment #443869 - Flags: review?(jst)
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.
And to clarify, the patch is for m-c, not for e10s branch.
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.
(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.
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!
Thanks for testing. I'll try to fix the tryserver failures today/tomorrow and
I'll probably change the script loading to happen synchronously.
Created attachment 444389 [details] [diff] [review]

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.
Attachment #443872 - Attachment is obsolete: true
Attachment #444389 - Flags: review?(jst)
Attachment #443872 - Flags: review?(jst)
Locally chrome tests pass now, but I posted the patch anyway to tryserver.
Uh, tryserver doesn't like the patch. Investigating.
Created attachment 444478 [details] [diff] [review]

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.
Attachment #444389 - Attachment is obsolete: true
Attachment #444478 - Flags: review?(jst)
Attachment #444389 - Flags: review?(jst)
The latest patch seems to have passed tests on tryserver, though
some random looking orange in some builds.
Comment on attachment 444478 [details] [diff] [review]

Bah, I found still something to fix. And chrome tests are coming too.
Attachment #444478 - Flags: review?(jst)
Created attachment 444767 [details] [diff] [review]

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).
Attachment #444478 - Attachment is obsolete: true
Attachment #444767 - Flags: review?(jst)
Comment on attachment 444767 [details] [diff] [review]

Tryserver liked this better :)
Comment on attachment 444767 [details] [diff] [review]

Looks good.
Attachment #444767 - Flags: review?(jst) → review+
But let's see if that works before marking this fixed.
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?
Indeed. Thanks!

I'll fix in a followup hg push, if the patch keeps tbox green.
Backed out. There is some crash.
Created attachment 445939 [details] [diff] [review]
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.
Only known oranges. Marking fixed.
Please open new bugs for issues and feature requests.
Last Resolved: 7 years ago
Resolution: --- → FIXED
smaug, these changes make for a tricky merge of m-c to e10s.  Do you mind doing it?

Comment 33

7 years ago
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?
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.
(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.

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)
Depends on: 567673
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().
jsapi.h docs are on MDC, not in the .h file itself; see specifically

See also

jst and mrbkap know the drill; so do others.


Comment 39

7 years ago
This is what I spotted:

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

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);

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

Comment 40

7 years ago
+    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?
Depends on: 574049
Depends on: 578960
Depends on: 582441

Comment 41

7 years ago
Keywords: dev-doc-needed
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:

Also added the following reference pages:
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.