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

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[michael.rouges]

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

[Andre Klapper]

What does this have to do with Firefox?

Comment 2

[michael.rouges]

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.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Maybe a Core::Javascript Engine issue? 
Possibly relevant: https://developer.mozilla.org/en-US/docs/Web/API/ErrorEvent

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Actually, this is a general problem with error events, not worker-specific.  Patch coming up once I finish writing the tests.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

bz, so awesome. Thank you!

Comment 7

[michael.rouges]

Thanks all =)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Generate virtual As*Event functions on WebIDL events so that we can easily go from an nsDOMEvent to the relevant event subtype.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Get rid of InternalScriptErrorEvent and just use mozilla::dom::ErrorEvent for the cases that used to use it.

I couldn't figure out a sane way to split this up into smaller pieces, unfortunately

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 with diff -w to make review a bit easier for that prescontext null-check removal bit

Comment 11

[Boris Zbarsky [:bzbarsky]]

Michael, thank you for filing this!

Comment 12

[Boris Zbarsky [:bzbarsky]]

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 13

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

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

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

Indeed. I should have looked at some more context.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 with that fixed and the AutoRestore bit

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff

Comment 21

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/837481767455
   https://hg.mozilla.org/integration/mozilla-inbound/rev/24aa8fa1f13a

Comment 22

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/016b9229193e because I wasn't bright enough to figure out whether it was this or bug 973849 causing Windows builds to say https://tbpl.mozilla.org/php/getParsedLog.php?id=35109146&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=35109059&tree=Mozilla-Inbound

Comment 23

[Boris Zbarsky [:bzbarsky]]

It was this one.  Allow me to once again curse windows.h.  :(

Comment 24

[Boris Zbarsky [:bzbarsky]]

Also, shows me what I get for doing a not-all-platforms try push.  :(

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm tempted to do what we did in nsGlobalWindow.cpp all over dom/.

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Certainly we should do it in the generated binding headers.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Er, cpps, of course.

Comment 28

[Boris Zbarsky [:bzbarsky]]

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.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> 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....

Comment 30

[Boris Zbarsky [:bzbarsky]]

OK.  So it looks like some unified file that comes before nsJSEventListener.cpp is including windows.h and screwing over all later files.  :(

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 32

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 :(

Comment 33

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 34

[Boris Zbarsky [:bzbarsky]]

Kyle, any idea what msvc is doing with NS_HandleScriptError there?

Comment 35

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 36

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 37

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should consider making C4099 fatal, IMO.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> class vs. struct confusion is the only thing I can think of.

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

Comment 39

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix Windows build issues

Comment 40

[(no longer active)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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] ([e-mail]) from comment #37)
> We should consider making C4099 fatal, IMO.

Agreed.

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 41

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1851a02b1ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/73abdca84f67

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b1851a02b1ca
https://hg.mozilla.org/mozilla-central/rev/73abdca84f67

Comment 43

[michael.rouges]

Gratz for your efficiency... thanks guys. :D

Comment 44

[Boris Zbarsky [:bzbarsky]]

No problem.  Thanks for filing the bug!

Comment 45

[michael.rouges]

Not knowing the language used, acutally, it's now all that I can make, but I'make it with pleasure. ;)