Closed
Bug 598438
Opened 14 years ago
Closed 14 years ago
window.console and window.onerror fail to work in iframes on first Web Console open
Categories
(DevTools :: General, defect, P3)
DevTools
General
Tracking
(Not tracked)
RESOLVED
WORKSFORME
People
(Reporter: msucan, Assigned: msucan)
References
Details
(Whiteboard: [patchclean:1118])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Gavin
:
review-
|
Details | Diff | Splinter Review |
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•14 years ago
|
Assignee: nobody → mihai.sucan
Blocks: devtools4b8
Assignee | ||
Comment 1•14 years ago
|
||
This is the proposed fix and test case.
Attachment #481461 -
Flags: feedback?(ddahl)
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [patchclean:1007]
Updated•14 years ago
|
Comment 2•14 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•14 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•14 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)
Comment 5•14 years ago
|
||
I'd like it if this were obsoleted by bug 587734 - is that the case?
Assignee | ||
Comment 6•14 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.
Comment 7•14 years ago
|
||
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•14 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)
Comment 9•14 years ago
|
||
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•14 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. ;)
Updated•14 years ago
|
blocking2.0: ? → betaN+
Comment 11•14 years ago
|
||
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)
Updated•14 years ago
|
URL: 605241,587734
Updated•14 years ago
|
Updated•14 years ago
|
URL: 605241,587734
Updated•14 years ago
|
Assignee | ||
Comment 12•14 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•14 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•14 years ago
|
||
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)
Comment 15•14 years ago
|
||
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 16•14 years ago
|
||
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•14 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 20•14 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•14 years ago
|
||
Kevin: Yes, that is correct. I am still waiting for Gavin's reply. ;)
blocking2.0: betaN+ → ---
Updated•14 years ago
|
Severity: blocker → normal
Priority: P1 → P3
Assignee | ||
Comment 22•14 years ago
|
||
Code has changed a lot and the Web Console is no longer affected by this bug.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•