Closed Bug 1066340 Opened 10 years ago Closed 10 years ago

Errors thrown from message manager callbacks don't trigger window.onerror

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bholley, Assigned: bholley)

Details

Attachments

(2 files)

While hacking on test_cpows.xul, I noticed that I could introduce silly syntax all over the place and the test would still pass. As it turns out, this is a property of all message handlers in gecko right now. Yuck.

It looks like I probably broke this in mozilla25 with https://hg.mozilla.org/mozilla-central/rev/329b420eb34b

Thankfully, due to the recent JSAPI refactoring, we're now in a much better place to fix this than we were, and I wrote up a patch with tests. Let's see how many uncaught errors have accumulated in the meantime.
Without this, we fail in [1] and friends when we pass the host value to
asyncResolve.

[1] browser/base/content/test/general/browser_keywordSearch.js
Attachment #8488952 - Flags: review?(gijskruitbosch+bugs)
(In reply to Bobby Holley (:bholley) from comment #4)
> https://tbpl.mozilla.org/?tree=Try&rev=0bcef7823d07

Full try push is green.
Comment on attachment 8488952 [details] [diff] [review]
Part 1 - Handle null/undefined alternativeURI.host. v1

Review of attachment 8488952 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh. I didn't even think that this was possible for http URIs! :-\
Attachment #8488952 - Flags: review?(gijskruitbosch+bugs) → review+
Attachment #8488953 - Flags: review?(bugs) → review+
Comment on attachment 8488953 [details] [diff] [review]
Part 2 - Use an AutoEntryScript in nsFrameMessageManager::ReceiveMessage. v1

Review of attachment 8488953 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsFrameMessageManager.cpp
@@ +962,5 @@
> +
> +      // Note - The ergonomics here will get a lot better with bug 971673:
> +      //
> +      // AutoEntryScript aes;
> +      // if (!aes.Init(wrappedJS->GetJSObject())) {

Won't we need an nsIGlobalObject for the Init?
(In reply to Bob Owen (:bobowen) from comment #8)
> Won't we need an nsIGlobalObject for the Init?

I think we should add a helper Init that does that step automatically. I thought it was part of jimb's patch, but maybe not.
(In reply to Bobby Holley (:bholley) from comment #9)
> (In reply to Bob Owen (:bobowen) from comment #8)
> > Won't we need an nsIGlobalObject for the Init?
> 
> I think we should add a helper Init that does that step automatically. I
> thought it was part of jimb's patch, but maybe not.

Ah, OK, it certainly makes things look tidier.
I think there were a couple cases where it would have been useful for AutoJSAPI as well.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: