Event dispatched to the global for script errors is not an ErrorEvent

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: michael.rouges, Assigned: bzbarsky)

Tracking

28 Branch
mozilla30
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox28 affected, firefox29- affected, firefox30- affected)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140205162153

Steps to reproduce:

I'm trying to create a worker from a dataURL, that, it's ok.

Then, I try to catch an error, inside the worker, by an event handler.

http://jsfiddle.net/myQB3/


Actual results:

The ErrorEvent only contains 1 property : isTrusted


Expected results:

message, filename, lineno fields are missing but expected.

Comment 1

5 years ago
What does this have to do with Firefox?
Flags: needinfo?(michael.rouges)
Reporter

Comment 2

5 years ago
In my view, because Firefox allows a worker to intercept an error, the event handler must return an Error or ErrorEvent instance, with the properties of this error.

In the current state, this lack of consistency and greatly reduces the potential for error debugging.
Flags: needinfo?(michael.rouges)
Maybe a Core::Javascript Engine issue? 
Possibly relevant: https://developer.mozilla.org/en-US/docs/Web/API/ErrorEvent
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
> The ErrorEvent only contains 1 property : isTrusted

A few things going on here:

1)  You're using the wrong API to get the property names.  getOwnPropertyNames only gets _own_ properties, but most properties of most DOM objects live on the prototype.  isTrusted is an exception for events, because it's marked unforgeable in the IDL.  I've put a fixed version of your fiddle at http://jsfiddle.net/myQB3/1/ that actually shows all the property names.

2)  The event being fired is in fact not an ErrorEvent; looking into why now.
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → DOM: Workers
Ever confirmed: true
Actually, this is a general problem with error events, not worker-specific.  Patch coming up once I finish writing the tests.
Assignee: nobody → bzbarsky
Component: DOM: Workers → DOM: Events
Summary: Worker internal onerror fails on error data → Event dispatched to the global for script errors is not an ErrorEvent
bz, so awesome. Thank you!
Reporter

Comment 7

5 years ago
Thanks all =)
I couldn't figure out a sane way to split this up into smaller pieces, unfortunately
Attachment #8376536 - Flags: review?(bugs)
Attachment #8376535 - Flags: review?(bugs) → review+
Michael, thank you for filing this!
https://tbpl.mozilla.org/?tree=Try&rev=63608bfeb167 says that some devtools tests are not getting uncaught exceptions when they should be... unclear why.  In general, the uncaught exception machinery in mochitest works with this patch; I did check that.
Comment on attachment 8376536 [details] [diff] [review]
part 2.  Get rid of InternalScriptErrorEvent and just use mozilla::dom::ErrorEvent for the cases that used to use it.

>         sHandlingScriptError = true; // Recursion prevention
> 
>         nsRefPtr<nsPresContext> presContext;
>         docShell->GetPresContext(getter_AddRefs(presContext));
> 
>-        if (presContext) {
Is it possible that because of this change sHandlingScriptError stays true in some case,
and devtool tests don't get the expected error.
sHandlingScriptError could use AutoRestore.

>     NS_ENSURE_TRUE(aEvent, NS_ERROR_UNEXPECTED);
>-    InternalScriptErrorEvent* scriptEvent =
>-      aEvent->GetInternalNSEvent()->AsScriptErrorEvent();
>-    if (scriptEvent &&
>-        (scriptEvent->message == NS_LOAD_ERROR ||
>-         scriptEvent->typeString.EqualsLiteral("error"))) {
>-      errorMsg = scriptEvent->errorMsg;
>+    ErrorEvent* scriptEvent = aEvent->InternalDOMEvent()->AsErrorEvent();
>+    if (scriptEvent) {
>+      scriptEvent->GetMessage(errorMsg);
>       msgOrEvent.SetAsString().SetData(errorMsg.Data(), errorMsg.Length());
So if one have non-onerror event handler, and error event is redispatched with different type, this
sure does something unexpected.
So, please tests that we have sane type.
(Want to add a small test for that too.)
Attachment #8376536 - Flags: review?(bugs) → review-
> Is it possible that because of this change sHandlingScriptError stays true in some case,

No.  I tried reverting that part of this patch and it didn't affect devtools.  Or the other browser-chrome tests that fail, actually (there are a few non-devtools ones).

> and error event is redispatched with different type

I'm not sure what you mean here.
Flags: needinfo?(bugs)
What Olli meant was:

  element.dispatchEvent(new ErrorEvent("click"))

which should not trigger the multi-arg form of window.onerror.  But afaict the relevant code is inside this block:

  if (mHandler.Type() == nsEventHandler::eOnError) {
    MOZ_ASSERT_IF(mEventName, mEventName == nsGkAtoms::onerror);

so we should never reach it anyway in the new ErrorEvent("click") case.
Indeed. I should have looked at some more context.
Flags: needinfo?(bugs)
OK, the test failure mystery is solved.  Our current script error events bubble.  My new ones do not, per spec.  But the browser-chrome tests rely on them bubbling to the chrome event handler and hence to the browser window, which is where the onerror listener is.

I'll just make the events bubble again for now, and filed bug 974548.
Attachment #8376536 - Attachment is obsolete: true
Comment on attachment 8378466 [details] [diff] [review]
Part 2 with that fixed and the AutoRestore bit

Try is green now.  I'll post an interdiff in a sec.
Attachment #8378466 - Flags: review?(bugs)
Attachment #8376538 - Attachment is obsolete: true
Attachment #8378466 - Flags: review?(bugs) → review+
It was this one.  Allow me to once again curse windows.h.  :(
Also, shows me what I get for doing a not-all-platforms try push.  :(
I'm tempted to do what we did in nsGlobalWindow.cpp all over dom/.
Certainly we should do it in the generated binding headers.
So the second bustage there was from IDBRequest.cpp not actually having a "using mozilla::dom" and presumably from clang and msvc disagreeing on namespace lookup rules or something.

The first bustage has two parts.  The first is:

nsGlobalWindow.obj : error LNK2019: unresolved external symbol "bool __cdecl NS_HandleScriptError(class nsIScriptGlobalObject *,class mozilla::dom::ErrorEventInit const &,enum nsEventStatus *)" (?NS_HandleScriptError@@YA_NPAVnsIScriptGlobalObject@@ABVErrorEventInit@dom@mozilla@@PAW4nsEventStatus@@@Z) referenced in function "public: virtual enum tag_nsresult __thiscall nsIScriptGlobalObject::HandleScriptError(class mozilla::dom::ErrorEventInit const &,enum nsEventStatus *)" (?HandleScriptError@nsIScriptGlobalObject@@UAE?AW4tag_nsresult@@ABVErrorEventInit@dom@mozilla@@PAW4nsEventStatus@@@Z)

but the function is in fact defined, in nsJSEnvironment.h.  Is MSVC getting confused because the declaration explicitly says "mozilla::dom::ErrorEventInit" but the definition says "ErrorEventInit" (but in a file with "using namespace mozilla::dom;")?  Kyle, any idea what's up here?

The second part is the windows.h nasties.  I'll do some try runs to hunt this down.
> Certainly we should do it in the generated binding headers.
> Er, cpps, of course.

We already do that.  The binding unified files have stuff like this in them:

  #include "AbstractWorkerBinding.cpp"
  #ifdef _WINDOWS_
  #error "AbstractWorkerBinding.cpp included windows.h"
  #endif

etc.  But in this case, the windows.h bustage is:

Unified_cpp_dom_events2.obj : error LNK2019: unresolved external symbol "public: void __thiscall mozilla::dom::ErrorEvent::GetMessageW(class nsString &)const " (?GetMessageW@ErrorEvent@dom@mozilla@@QBEXAAVnsString@@@Z) referenced in function "public: virtual enum tag_nsresult __stdcall nsJSEventListener::HandleEvent(class nsIDOMEvent *)" (?HandleEvent@nsJSEventListener@@UAG?AW4tag_nsresult@@PAVnsIDOMEvent@@@Z)

so it's not a binding file.  :(  I'd _love_ to sprinkle that magic all over dom/, though....
OK.  So it looks like some unified file that comes before nsJSEventListener.cpp is including windows.h and screwing over all later files.  :(
Aha!  nsDOMUIEvent.cpp includes ipc/IPCMessageUtils.h which includes "base/process_util.h" (ipc/chromium/src/base/process_util.h), which includes <windows.h>.

Kyle, can we just #undef all the things that need undeffing in IPCMessageUtils.h?  Or should we pull nsDOMUIEvent out of the unified build?

Note that nsDOMEvent also includes IPCMessageUtils.h, but that one is not in the same unified file as nsJSEventListener.cpp.

And none of that explains the NS_HandleScriptError unresolved symbol error.  :(
Flags: needinfo?(khuey)
I think we should pull anything that includes IPCMessageUtils.h out of the unified build.

Slightly longer term, I think we should make IPCMessageUtils.h not (transitively) include windows.h.  I think this should be doable, and it'll stop this nasty problem of anything that wants to work in e10s polluting our world :(
Flags: needinfo?(khuey)
Also, looks like IPCMessageUtils is included by nsDOMNotifyPaintEvent.  And, via HalSensor.h, in nsEventListenerManager.cpp.

By the way, I tried explicitly namespacing the arg in the definition of NS_HandleScriptError.  https://tbpl.mozilla.org/?tree=Try&rev=9ecb27bce5b1 says that still fails.  :(
Kyle, any idea what msvc is doing with NS_HandleScriptError there?
Flags: needinfo?(khuey)
class vs. struct confusion is the only thing I can think of.  The generated thing is a struct, and MSVC is more pedantic about the class/struct distinction than other compilers (though it only warns about fwd decls in the compiler, afaik).
Flags: needinfo?(khuey)
And according to http://en.wikipedia.org/wiki/Visual_C%2B%2B_name_mangling#Data_Type, structs and classes are mangled differently (see the 'U' vs 'V' bit).

That's nasty.
We should consider making C4099 fatal, IMO.
> class vs. struct confusion is the only thing I can think of.

Aha, bingo!  Patch coming up that makes this compile on Windows.

Comment 40

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #35)
> class vs. struct confusion is the only thing I can think of.  The generated
> thing is a struct, and MSVC is more pedantic about the class/struct
> distinction than other compilers (though it only warns about fwd decls in
> the compiler, afaik).

Yes, I have fixed at least 5 of these types of bugs.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #37)
> We should consider making C4099 fatal, IMO.

Agreed.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #32)
> I think we should pull anything that includes IPCMessageUtils.h out of the
> unified build.

I don't think that we should do this.  We currently have around 800 source files that rely on that header.
Reporter

Comment 43

5 years ago
Gratz for your efficiency... thanks guys. :D
No problem.  Thanks for filing the bug!
Reporter

Comment 45

5 years ago
Not knowing the language used, acutally, it's now all that I can make, but I'make it with pleasure. ;)
Duplicate of this bug: 795246
You need to log in before you can comment on or make changes to this bug.