Girish Sharma [:Optimizer] Reporter
	Description • 12 years ago

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.

	Girish Sharma [:Optimizer] Reporter
	Comment 1 • 12 years ago

To add to it, after the exceptions are thrown, opening debugger fails. Debugger does not get any script.

	Heather Arthur [:harth]
	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).

	Panos Astithas (he/him) [:past] (please ni?) 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.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 4 • 12 years ago

I'll try to provide a fix for this.

	Panos Astithas (he/him) [:past] (please ni?) 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.

	Joe Walker [: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 &&
        ...

?

	Panos Astithas (he/him) [:past] (please ni?) 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.

	Joe Walker [:jwalker]
	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?

	Panos Astithas (he/him) [:past] (please ni?) 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.

	Heather Arthur [:harth]
	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.

	Joe Walker [:jwalker]
	Comment 11 • 12 years ago

Comment on attachment 730825 [details] [diff] [review]
Patch v2

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

Thanks Panos.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 12 • 12 years ago

https://hg.mozilla.org/integration/fx-team/rev/98e19635eb97

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 13 • 12 years ago

Backed out due to oranges:
https://hg.mozilla.org/integration/fx-team/rev/13c4b1ff5564

	Panos Astithas (he/him) [:past] (please ni?) 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.

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 15 • 12 years ago

Squashed and relanded:
https://hg.mozilla.org/integration/fx-team/rev/86daec0a6742

	Victor Porof [:vporof][:vp]
	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

	Panos Astithas (he/him) [:past] (please ni?) 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

	Panos Astithas (he/him) [:past] (please ni?) Assignee
	Comment 19 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/6547ddfae944

	Ioana (away)
	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?

	Panos Astithas (he/him) [:past] (please ni?) 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.