Closed
Bug 849500
Opened 12 years ago
Closed 12 years ago
navigate and will-navigate events for remoted targets carry payload that is incompatible with the non-remoted case
Categories
(DevTools :: Framework, defect, P1)
Tracking
(firefox21 unaffected, firefox22+ verified)
RESOLVED
FIXED
Firefox 23
Tracking | Status | |
---|---|---|
firefox21 | --- | unaffected |
firefox22 | + | verified |
People
(Reporter: Optimizer, Assigned: past)
References
Details
(Whiteboard: [fixed-in-fx-team])
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
jwalker
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwalker
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
In the latest fx-team, Open inspector, switch to web console, reload page. You will see two errors in error console: Timestamp: 3/9/2013 3:17:46 PM Error: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [inIDOMUtils.clearPseudoClassLocks]" nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)" location: "JS frame :: resource:///modules/devtools/InspectorPanel.jsm :: InspectorPanel_clearPseudoClasses/< :: line 636" data: no]: undefined Source File: resource:///modules/devtools/EventEmitter.jsm Line: 105 Timestamp: 3/9/2013 3:17:46 PM Error: TypeError: newWindow.document is undefined: InspectorPanel_onNavigatedAway@resource:///modules/devtools/InspectorPanel.jsm:261 EventEmitter_emit@resource:///modules/devtools/EventEmitter.jsm:100 onRemoteTabNavigated@resource:///modules/devtools/Target.jsm:302 EV_notify@resource://gre/modules/devtools/dbg-client.jsm:147 DC_onPacket/<@resource://gre/modules/devtools/dbg-client.jsm:506 effort@resource://gre/modules/commonjs/sdk/core/promise.js:55 resolved@resource://gre/modules/commonjs/sdk/core/promise.js:117 then@resource://gre/modules/commonjs/sdk/core/promise.js:37 then@resource://gre/modules/commonjs/sdk/core/promise.js:123 DC_onPacket@resource://gre/modules/devtools/dbg-client.jsm:517 LDT_send/<.run@chrome://global/content/devtools/dbg-transport.js:224 Source File: resource:///modules/devtools/EventEmitter.jsm Line: 105 What is happening is that the navigateaway notification from the server is coming too early that the window.document is not present at that point, thus Inspector fails to load after that. Which means that if you switch back to inspector after the reload is complete, you will not see the markup view. This might be a regression because of bug 820524 as I cannot reproduce the same errors in latest nightly.
Reporter | ||
Updated•12 years ago
|
Severity: normal → major
Priority: -- → P1
Reporter | ||
Comment 1•12 years ago
|
||
To add to it, after the exceptions are thrown, opening debugger fails. Debugger does not get any script.
Comment 2•12 years ago
|
||
I think I'm running into this working on the remote Style Editor. It happens only after you've called target.makeRemote() on the TabTarget (e.g. after you've opened the web console or debugger).
Assignee | ||
Comment 3•12 years ago
|
||
What a mess, how come we didn't have tests for this? The problem is that remote targets cannot keep a reference to the content window or document, so the navigate/will-navigate events only provide the protocol packet as a parameter. This means that we either need to make the style tools a little remote-aware or partially undo the unification of TabTarget and RemoteTarget. Heather I assume the first option would be better for your style editor work.
Summary: navigate away event firing too early. → navigate and will-navigate events for remoted targets carry payload that is incompatible with the non-remoted case
Assignee | ||
Comment 4•12 years ago
|
||
I'll try to provide a fix for this.
Assignee: nobody → past
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•12 years ago
|
||
This patch fixes the problem by: (a) keeping the WebProgressListener around even after the target is remoted (b) making the WebProgressListener store the former navigation event payload when the target is remoted instead of transmitting it directly (c) making the remote target navigation event handler send both the new and any stored payload All tests pass and my manual testing didn't show any problems, but I hope others can test the style tools more thoroughly. It's a hacky solution, but supporting both remotable and non-remotable tools with the same Target is already hacky. There are two cases (in InspectorPanel.jsm and Selection.jsm) where I've silenced errors being thrown on navigation, that could be improved though. One of them seems identical to bug 848731, but with different STR, but both are about dead objects being accessed. From what I can tell the problem is that in both places the code executes cleanup and initialization activities on the |navigate| event, instead of doing the cleanup work on |will-navigate| and the initialization on |navigate|. Combining them doesn't work any more because the new |navigate| event is triggered slightly later than before, when the DOM of the previous page is already gone. I'm not sure whether it's better to do the better fix now or wait for the remote inspector to land. Opinions are welcome. I would note though that I may not be the best person to make these changes if we want to get a fix in before the next uplift. I can work on it in a followup though, if nobody else is available. The same holds for adding a test that makes sure the inspector works fine after navigation, since we apparently lack such a test.
Attachment #730689 -
Flags: review?(jwalker)
Comment 6•12 years ago
|
||
Comment on attachment 730689 [details] [diff] [review] Patch v1 Review of attachment 730689 [details] [diff] [review]: ----------------------------------------------------------------- Correct me if my thinking is broken here: Perhaps it was a mistake to pass the window to the navigate event handlers. They could find the information out another way. Also having the packet passed for remote event handlers made matters worse be because now there are 2 conflicting definitions of the event. Can we easily get rid of both the window and the packet? ::: browser/devtools/inspector/Selection.jsm @@ +161,5 @@ > }, > > isNode: function SN_isNode() { > + let result = false; > + try { Could you also do: if (!Components.utils.isDeadWrapper(this.node) && this.node && ... ?
Attachment #730689 -
Flags: review?(jwalker)
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Joe Walker [:jwalker] from comment #6) > Correct me if my thinking is broken here: Perhaps it was a mistake to pass > the window to the navigate event handlers. They could find the information > out another way. Also having the packet passed for remote event handlers > made matters worse be because now there are 2 conflicting definitions of the > event. > > Can we easily get rid of both the window and the packet? I don't know if the style tools could live without a window parameter, but the remotable tools need access to information about the new page (URL, title, etc.) and there is no other safe way to get it. The only way to get this data is from a listTabs request which can be slow when many tabs are open, plus the client would not be aware of the previous state of open tabs to surmise the changed one. Not to mention the potential races from multiple navigation events in quick succession, or the inefficiency of having to hit the network before the tools are updated (think mobile devices). Would it look better if I made the DOMWindow/nsIRequest a property of the packet, leaving the event handler signatures unchanged? > ::: browser/devtools/inspector/Selection.jsm > @@ +161,5 @@ > > }, > > > > isNode: function SN_isNode() { > > + let result = false; > > + try { > > Could you also do: > > if (!Components.utils.isDeadWrapper(this.node) && > this.node && > ... > > ? I can try that.
Comment 8•12 years ago
|
||
(In reply to Panos Astithas [:past] from comment #7) > (In reply to Joe Walker [:jwalker] from comment #6) > > Correct me if my thinking is broken here: Perhaps it was a mistake to pass > > the window to the navigate event handlers. They could find the information > > out another way. Also having the packet passed for remote event handlers > > made matters worse be because now there are 2 conflicting definitions of the > > event. > > > > Can we easily get rid of both the window and the packet? > > I don't know if the style tools could live without a window parameter, but > the remotable tools need access to information about the new page (URL, > title, etc.) and there is no other safe way to get it. The only way to get > this data is from a listTabs request which can be slow when many tabs are > open, plus the client would not be aware of the previous state of open tabs > to surmise the changed one. Not to mention the potential races from multiple > navigation events in quick succession, or the inefficiency of having to hit > the network before the tools are updated (think mobile devices). > > Would it look better if I made the DOMWindow/nsIRequest a property of the > packet, leaving the event handler signatures unchanged? What information does the new page do we need? Could we pass an event object with { title, ... url: ... } properties?
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Joe Walker [:jwalker] from comment #8) > (In reply to Panos Astithas [:past] from comment #7) > > Would it look better if I made the DOMWindow/nsIRequest a property of the > > packet, leaving the event handler signatures unchanged? > > What information does the new page do we need? Could we pass an event object > with { title, ... url: ... } properties? I did both of these and also made the isDeadWrapper change.
Attachment #730689 -
Attachment is obsolete: true
Attachment #730825 -
Flags: review?(jwalker)
Comment 10•12 years ago
|
||
The(In reply to Panos Astithas [:past] from comment #3) > What a mess, how come we didn't have tests for this? > > The problem is that remote targets cannot keep a reference to the content > window or document, so the navigate/will-navigate events only provide the > protocol packet as a parameter. This means that we either need to make the > style tools a little remote-aware or partially undo the unification of > TabTarget and RemoteTarget. > > Heather I assume the first option would be better for your style editor work. For the remote Style Editor, it doesn't matter if the target's navigate event doesn't pass the content window. Actually, it turns out that my bug was different than this one. I'm not getting any 'navigate' or 'will-navigate' events after calling makeRemote(). I only start receiving them after the web console or debugger is opened. I don't know if it's related to this bug.
Comment 11•12 years ago
|
||
Comment on attachment 730825 [details] [diff] [review] Patch v2 Review of attachment 730825 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Panos.
Attachment #730825 -
Flags: review?(jwalker) → review+
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/98e19635eb97
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 13•12 years ago
|
||
Backed out due to oranges: https://hg.mozilla.org/integration/fx-team/rev/13c4b1ff5564
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 14•12 years ago
|
||
The fix was almost trivial once I realized what the problem was. I ran the tests a million times yesterday in various patch queue combinations, in preperation for the push, but I was just looking for red in mach output, so I was bitten by bug 857984.
Attachment #730825 -
Attachment is obsolete: true
Attachment #733308 -
Flags: review?(jwalker)
Updated•12 years ago
|
Attachment #733308 -
Flags: review?(jwalker) → review+
Assignee | ||
Comment 15•12 years ago
|
||
Squashed and relanded: https://hg.mozilla.org/integration/fx-team/rev/86daec0a6742
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Updated•12 years ago
|
Attachment #730825 -
Attachment is obsolete: false
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/98e19635eb97 https://hg.mozilla.org/mozilla-central/rev/13c4b1ff5564 https://hg.mozilla.org/mozilla-central/rev/86daec0a6742
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Assignee | ||
Updated•12 years ago
|
status-firefox22:
--- → affected
tracking-firefox22:
--- → ?
Assignee | ||
Comment 18•12 years ago
|
||
[Approval Request Comment] Bug caused by (feature/regressing bug #): bug 820524 User impact if declined: switching between some developer tools and reloading the page will make the page inspector and style editor not work as expected Testing completed (on m-c, etc.): m-c and aurora (locally) Risk to taking this patch (and alternatives if risky): patch only touches devtools code, so it should pose no risk to the general population String or IDL/UUID changes made by this patch: none
Attachment #735718 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
status-firefox21:
--- → unaffected
Updated•12 years ago
|
Attachment #735718 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 20•12 years ago
|
||
Verified as fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (20130514181517). This issue doesn't reproduce anymore with regular sites (google.com, yahoo.com etc). Part of the issue does still reproduce with in-content pages. While performing the steps from comment 0 and reloading about:home, I got the following: Timestamp: 5/16/2013 3:08:00 PM Error: TypeError: newWindow.document is undefined: InspectorPanel_onNavigatedAway@resource://app/modules/devtools/InspectorPanel.jsm:272 EventEmitter_emit@resource:///modules/devtools/EventEmitter.jsm:100 onRemoteTabNavigated@resource://app/modules/devtools/Target.jsm:303 EV_notify@resource://gre/modules/devtools/dbg-client.jsm:147 @resource://gre/modules/devtools/dbg-client.jsm:509 resolve@resource://gre/modules/commonjs/sdk/core/promise.js:120 then@resource://gre/modules/commonjs/sdk/core/promise.js:45 then@resource://gre/modules/commonjs/sdk/core/promise.js:155 DC_onPacket@resource://gre/modules/devtools/dbg-client.jsm:521 @chrome://global/content/devtools/dbg-transport.js:224 Source File: resource:///modules/devtools/EventEmitter.jsm Line: 105 I this expected or should I file a new bug for it?
Updated•12 years ago
|
Assignee | ||
Comment 21•12 years ago
|
||
(In reply to Ioana Budnar, QA [:ioana] from comment #20) > I this expected or should I file a new bug for it? I could only reproduce it in about:home, so we don't need to worry about it. It's already fixed in Nightly.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•