598438 - window.console and window.onerror fail to work in iframes on first Web Console open

Status: RESOLVED WORKSFORME

(DevTools :: General, defect, P3)

Description

[Mihai Sucan [:msucan]]

STR:

1. make a page with an iframe that has a script which throws exceptions and/or invokes window.console methods.
2. open the page.
3. open the web console.
4. trigger the code in the iframe which throws exceptions and/or invokes window.console methods.

Actual result: nothing shows in the web console.

Expected result: the window.onerror event handler displays the exceptions thrown by the iframe, and the window.console messages.

If you reload the page while the Web Console is open, things work fine.

Problem is with the way the Web Console is initialized. It should run the windowInitializer() on each iframe window as well.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed fix and test

This is the proposed fix and test case.

Comment 2

[Kevin Dangoor]

Nominating as a blocker. Some sites use a fair number of iframes (gmail!) and not catching the errors on first run is a real bummer, especially given the simple and well-tested fix.

Comment 3

[David Dahl :ddahl]

Comment on attachment 481461 [details] [diff] [review]
proposed fix and test

Looks good. I am not sure but it may need to change slightly after the lazy console lands. Maybe/maybe not.

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 481461 [details] [diff] [review]
proposed fix and test

Thanks David for the feedback+! Asking for review.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

I'd like it if this were obsoleted by bug 587734 - is that the case?

Comment 6

[Mihai Sucan [:msucan]]

(In reply to comment #5)
> I'd like it if this were obsoleted by bug 587734 - is that the case?

I expect bug 587734 won't entirely obsolete this patch. We'll have to see.

What I expect is that 587734 will solve only the issue of using the console API in iframe, when you first open the Web Console. However, the window.onerror event handler still needs to be properly setup, as in this patch.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

OK, so that patch seems to indicate that we never setup the handler on subframes. So why do things work fine after a reload, per comment 0?

Comment 8

[Mihai Sucan [:msucan]]

(In reply to comment #7)
> OK, so that patch seems to indicate that we never setup the handler on
> subframes. So why do things work fine after a reload, per comment 0?

After reload it works, because we listen for the content-document-global-created event. (see the HUDWindowObserver)

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Bug 605241 will remove the onerror handlers, and bug 587734 will remove the need to manually set up window.console, so with those fixed I think this shouldn't be needed.

Comment 10

[Mihai Sucan [:msucan]]

(In reply to comment #9)
> Bug 605241 will remove the onerror handlers, and bug 587734 will remove the
> need to manually set up window.console, so with those fixed I think this
> shouldn't be needed.

Agreed, but we can't close this bug yet. We need to see how they all turn out. ;)

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 481461 [details] [diff] [review]
proposed fix and test

I'm pretty confident those will both land, so let's get this out of dietrich's queue in the mean time :)

Comment 12

[Mihai Sucan [:msucan]]

Retested this bug now that the lazy console patches landed and bug 597136 also landed.

Results:

- on first open exceptions are now properly caught.
- but window.console fails to show the messages. This happens because windowInitializer() is only invoked for the top window object of the page. The ConsoleAPIObserver cannot map the window ID of the iframe to the hudId.
- given how the mochitest is constructed, it also fails due to bug 613013 - that needs a different fix provided there.

Shall I update this patch accordingly?

Comment 13

[Kevin Dangoor]

Yes, if there's a problem distinct from bug 613013 that needs to be solved we should fix it here.

Comment 14

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch. This patch requires the patch from bug 613013.

Please note that 613013 almost fixes the issue reported here, but it does that only thanks to the fallback in getHudIdByWindow().

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 491551 [details] [diff] [review]
updated patch

I think we should be relying on the fallback for this - the window cache is just a shortcut, and we shouldn't be trying to populate it eagerly (and we might get rid of it altogether eventually).

We should probably get the test in either way though.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 491551 [details] [diff] [review]
updated patch

Looking just at the test, it looks overly complex (with executeSoons and click handlers and such). Can't you just open the console, then load pages in iframes that call console.log onload, and make sure that the output is present?

Comment 17

[Mihai Sucan [:msucan]]

(In reply to comment #16)
> Comment on attachment 491551 [details] [diff] [review]
> updated patch
> 
> Looking just at the test, it looks overly complex (with executeSoons and click
> handlers and such). Can't you just open the console, then load pages in iframes
> that call console.log onload, and make sure that the output is present?

I know it looks overly complex, and I can do what you suggested, but that wouldn't really test the problem this bug report is about.

The bug report is specifically about loading a web page with an iframe, while the Web Console is not open. After load, the user opens the Web Console. I needed a way to trigger the use of console.log() and an exception from each document (from the main document and from the iframe). I chose to do this with a clickable button.

If you want me to make changes to the test, please let me know. Thanks!

Comment 18

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 19

[Kevin Dangoor]

mass change: filter mail on BLOCKERSETTING

Comment 20

[Kevin Dangoor]

I believe this bug is now tests only, right? We should remove the blocking flag if that's the case.

Comment 21

[Mihai Sucan [:msucan]]

Kevin: Yes, that is correct. I am still waiting for Gavin's reply. ;)

Comment 22

[Mihai Sucan [:msucan]]

Code has changed a lot and the Web Console is no longer affected by this bug.