Closed Bug 849500 Opened 7 years ago Closed 7 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, major)

x86_64
Windows 7
defect

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)

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.
Severity: normal → major
Priority: -- → P1
Blocks: 820524
To add to it, after the exceptions are thrown, opening debugger fails. Debugger does not get any script.
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).
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
I'll try to provide a fix for this.
Assignee: nobody → past
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
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 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)
(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.
(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?
Attached patch Patch v2Splinter Review
(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)
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 on attachment 730825 [details] [diff] [review]
Patch v2

Review of attachment 730825 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Panos.
Attachment #730825 - Flags: review?(jwalker) → review+
Backed out due to oranges:
https://hg.mozilla.org/integration/fx-team/rev/13c4b1ff5564
Whiteboard: [fixed-in-fx-team]
Attached patch Orange fixSplinter Review
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)
Attachment #733308 - Flags: review?(jwalker) → review+
Squashed and relanded:
https://hg.mozilla.org/integration/fx-team/rev/86daec0a6742
Whiteboard: [fixed-in-fx-team]
Attachment #730825 - Attachment is obsolete: false
Duplicate of this bug: 850308
[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?
Attachment #735718 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
(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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.