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

RESOLVED WORKSFORME

Status

()

Firefox
Developer Tools
P3
normal
RESOLVED WORKSFORME
8 years ago
7 years ago

People

(Reporter: msucan, Assigned: msucan)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchclean:1118])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
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.

Updated

8 years ago
Assignee: nobody → mihai.sucan
Blocks: 594225
(Assignee)

Comment 1

8 years ago
Created attachment 481461 [details] [diff] [review]
proposed fix and test

This is the proposed fix and test case.
Attachment #481461 - Flags: feedback?(ddahl)
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
Whiteboard: [patchclean:1007]
status2.0: --- → ?

Comment 2

8 years ago
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.
blocking2.0: --- → ?

Comment 3

8 years ago
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.
Attachment #481461 - Flags: feedback?(ddahl) → feedback+
(Assignee)

Comment 4

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

Thanks David for the feedback+! Asking for review.
Attachment #481461 - Flags: review?(dietrich)
I'd like it if this were obsoleted by bug 587734 - is that the case?
(Assignee)

Comment 6

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

Comment 8

8 years ago
(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)
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.
(Assignee)

Comment 10

7 years ago
(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. ;)
blocking2.0: ? → betaN+
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 :)
Attachment #481461 - Flags: review?(dietrich)
status2.0: ? → ---
Depends on: 597136
No longer depends on: 605241
(Assignee)

Comment 12

7 years ago
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

7 years ago
Yes, if there's a problem distinct from bug 613013 that needs to be solved we should fix it here.
(Assignee)

Comment 14

7 years ago
Created attachment 491551 [details] [diff] [review]
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().
Attachment #481461 - Attachment is obsolete: true
Attachment #491551 - Flags: review?(gavin.sharp)
(Assignee)

Updated

7 years ago
Depends on: 613013
Whiteboard: [patchclean:1007] → [patchclean:1118]
No longer depends on: 613013
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.
Attachment #491551 - Flags: review?(gavin.sharp) → review-
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?
(Assignee)

Comment 17

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

7 years ago
mass change: filter on PRIORITYSETTING
Blocks: 593956
Priority: -- → P1

Comment 19

7 years ago
mass change: filter mail on BLOCKERSETTING
Severity: normal → blocker

Comment 20

7 years ago
I believe this bug is now tests only, right? We should remove the blocking flag if that's the case.
(Assignee)

Comment 21

7 years ago
Kevin: Yes, that is correct. I am still waiting for Gavin's reply. ;)
blocking2.0: betaN+ → ---

Updated

7 years ago
Severity: blocker → normal
Priority: P1 → P3
(Assignee)

Comment 22

7 years ago
Code has changed a lot and the Web Console is no longer affected by this bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.