860101 - payload._navPayload is a Window in 'will-navigate' event handler

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Heather Arthur [:harth]]

This is no reduced test cast, but here's my remote style editor branch: https://github.com/harthur/devtools-window/compare/14ebd04ad510c4be6eb6a7a6c8a49e08ba796825...15c2a78dfc10a0bae29e973c4ddfa6156ed99ea1.

Expected is that the payload is a nsIRequest object.

When the Style Editor is using a remote connection to the local tab, I can confirm that the target's navPayload is set to an nsIRequest, but overwritten by Target.onLocationChange() to be a DOMWindow before the 'will-navigate' event handler is called.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I can't reproduce what you are seeing in my testing. It seems that will-navigate is emitted before _navPayload is changed. This is the log I get after refreshing http://htmlpad.org/debugger/ (navigate/will-navigate are emitted when we see _onTabNavigated printed):

HEATHER: setting _navPayload to request
DBG-SERVER: Got: {
  "from": "conn0.tab3",
  "type": "tabNavigated",
  "url": "http://htmlpad.org/debugger/",
  "nativeConsoleAPI": true,
  "state": "start"
}
HEATHER: _onTabNavigated
HEATHER: before navigate
HEATHER: before navigate isDirty false
HEATHER: arg [xpconnect wrapped nsIRequest @ 0x1004ea890 (native @ 0x10a959800)]
HEATHER: request[xpconnect wrapped nsIRequest @ 0x1004ea890 (native @ 0x10a959800)]
HEATHER: otherundefined

[...]

HEATHER: onLocationChange 
HEATHER: setting _navPayload to window

[...]

BG-SERVER: Got: {
  "from": "conn0.tab3",
  "type": "tabNavigated",
  "url": "http://htmlpad.org/debugger/",
  "title": "Debugger test page",
  "nativeConsoleAPI": true,
  "state": "stop"
}
HEATHER: _onTabNavigated


Do you have more specific STR for me to see the error? Should I be looking for something else/special in the log?

Perhaps more importantly though, I was under the impression that remoting the style editor would remove the need to use the _navPayload property of the event, since that will not be present in the remote case. What is the reason for continuing to use it in your patch?

Comment 2

[Heather Arthur [:harth]]

(In reply to Panos Astithas [:past] from comment #1)
> Do you have more specific STR for me to see the error? Should I be looking
> for something else/special in the log?

Ah sorry, I get this running a test, browser_styleeditor_bug_826982_location_changed.js.

> Perhaps more importantly though, I was under the impression that remoting
> the style editor would remove the need to use the _navPayload property of
> the event, since that will not be present in the remote case. What is the
> reason for continuing to use it in your patch?


I was hoping to use navPayload in the local case to pause the request when there are unsaved changes in the editor.

Comment 3

[Heather Arthur [:harth]]

Looks like it's a race condition. It happens when you navigate to a data url: 'data:text/html,<div></div>'

STR:

Run that remote style editor branch I linked to, pop open the Style Editor on a page and load 'data:text/html,<div></div>' in the urlbar.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v1

(In reply to Heather Arthur [:harth] from comment #3)
> Looks like it's a race condition. It happens when you navigate to a data
> url: 'data:text/html,<div></div>'

Darn, data URLs are weird. This patch fixes the problem by storing the request and window objects separately, so that the timing doesn't matter.

(In reply to Heather Arthur [:harth] from comment #2)
> (In reply to Panos Astithas [:past] from comment #1)
> > Perhaps more importantly though, I was under the impression that remoting
> > the style editor would remove the need to use the _navPayload property of
> > the event, since that will not be present in the remote case. What is the
> > reason for continuing to use it in your patch?
> 
> 
> I was hoping to use navPayload in the local case to pause the request when
> there are unsaved changes in the editor.

So I take that you are going to maintain separate code paths for the local and remote case? Also, isn't the difference in UX between the two cases of concern here?

In any case, if we wanted to implement something like that for the remote case, I think that we should add a new 'reconfigure' request to the tab actor (similar to the thread actor's) that will carry a pauseOnNavigation flag in the cases where the tools want to prompt the user first. That would probably require that we implement a generic mechanism for the tools to use first.

Comment 5

[Heather Arthur [:harth]]

(In reply to Panos Astithas [:past] from comment #4)
> So I take that you are going to maintain separate code paths for the local
> and remote case? Also, isn't the difference in UX between the two cases of
> concern here?

Yeah, I figured I would check for a request payload, and if it exists I could pause the request on unsaved changes. This wouldn't work in the remote case, But we can add something like you described in followup.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Patch v2

Rebased after the loader landing.

Comment 7

[Dave Camp (:dcamp)]

Seems like harth is a better reviewer here.

I had to back out the loader patch, so the first version is probably the one that should be reviewed.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 736162 [details] [diff] [review]
Patch v1

(In reply to Dave Camp (:dcamp) from comment #7)
> Seems like harth is a better reviewer here.
> 
> I had to back out the loader patch, so the first version is probably the one
> that should be reviewed.

Of course. I'm a moron.

Comment 9

[Heather Arthur [:harth]]

Comment on attachment 736162 [details] [diff] [review]
Patch v1

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

::: browser/devtools/framework/Target.jsm
@@ +507,1 @@
>      this.target = null;

null out navPayload as well?

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Heather Arthur [:harth] from comment #9)
> ::: browser/devtools/framework/Target.jsm
> @@ +507,1 @@
> >      this.target = null;
> 
> null out navPayload as well?

At this point the event object is no longer in scope and the progress listener does not keep any reference to it. Even if it did, it wouldn't be safe to null out the property while callers still have access to it.

I think event handlers already bear the burden of cleaning up after themselves, so I'm not worried about leaks from event._navPayload, to be honest.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/ec5efe533795