Closed Bug 1333352 Opened 7 years ago Closed 7 years ago

Navigation to the event listener source from the inspector is broken when using sourcemaps

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Version:
50 Branch
Type:
defect

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: stof, Assigned: tromey)

References

(Blocks 1 open bug)

Details

(Whiteboard: [todo-mr])

Attachments

(1 file)

Bug 1333352 - use client-side source map service in markup view event bubble;
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
Register an event listener in a file being sourcemapped and open the even listener bubble in the inspector. Then, click on the arrow to navigate to the Debugger panel.

Expected behavior: the source is displayed in the debugger

Actual behavior: the debugger panel displays a random source (the source injected by the ReactDevTools addon in my case)

This is kind of related to https://bugzilla.mozilla.org/show_bug.cgi?id=1325580 which is asking to display the original source. My request does not go that far (even though displaying the original source would be a better experience). What I would like is at least that the navigation itself work.
I'm not sure whether this is something to be reported against the Inspector panel or the Debugger panel (translating the request path after the navigation when source maps are applied and so the debugger displays original files rather than loaded files), so feel free to reassign the component as needed.
Hey Mike, could you please look into this a bit and find out whether the way the event tooltip gets the link to the source file is wrong, or whether the debugger panel should then resolve the location information to the right file?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mratcliffe)
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
I have looked into this before and Nick also did but we both failed to get event listeners working with sourcemaps... I can't recall exactly what the problem was.

This was some time back though so I don't mind revisiting it.
Flags: needinfo?(mratcliffe)
Whiteboard: [todo-mr]
Assignee: nobody → ttromey
Forgot to git add a file.
Blocks: source-maps
Comment on attachment 8867343 [details]
Bug 1333352 - use client-side source map service in markup view event bubble;

https://reviewboard.mozilla.org/r/138868/#review144612

Thanks for the patch Tom. I have a few minor comments and I found a small issue while testing which I want to check with you: the mapping no longer works if you close and reopen the toolbox.

STRs:
- go to https://juliandescottes.github.io/devtools-demos/inspector/event-sourcemap/ (took the liberty of uploading your test page)
- open inpector 
- click on event bubble, everything is correct
- close inspector
- open inspector
- click on event bubble, the sourcemapped location is not displayed, instead we have the bundle location

Note: if you got the debugger tab, and then go back to the inspector, then it works.
Maybe this is not related to this patch and is just a generic sourcemap issue? In this case feel free to ignore it and log it as a follow up. 
Is it the reason behind the ignore() function?

::: devtools/client/inspector/markup/test/browser_markup_events_source_map.js:8
(Diff revision 3)
> + * Test that the event details tooltip can be hidden by clicking outside of the tooltip
> + * after switching hosts.

nit: comment comes from another test, should be updated.

::: devtools/client/inspector/markup/test/browser_markup_events_source_map.js:38
(Diff revision 3)
> +      },
> +    ],
> +  },
> +];
> +
> +function ignore(x) { }

is this in order to call the getter for sourceMapURLService on toolbox and make sure it's not optimized out? 

(can getters actually be optimized out?)

In any case can you add a comment explaining why this is needed ?

::: devtools/client/shared/widgets/tooltip/EventTooltipHelper.js:125
(Diff revision 3)
> +            .then((originalLocation) => {
> +              if (originalLocation) {
> +                const { sourceUrl, line } = originalLocation;
> +                let newURI = sourceUrl + ":" + line;
> +                filename.textContent = newURI;
> +                state.uri = newURI;

I find this fragile sorry.

I am thinking that if we have to update the DOM, we might as well store the URI here. Well it's already in the textContent and the title, but maybe add it as data-attribute that we could read from debug-clicked? 

Not blocking the review on this, though.

::: devtools/client/shared/widgets/tooltip/EventTooltipHelper.js:280
(Diff revision 3)
>  
>        let matches = uri.match(/(.*):(\d+$)/);
> -      let line = 1;
>  
>        if (matches) {
> -        uri = matches[1];
> +        return {url: matches[1], line: parseInt(matches[2], 10)};

nit: I think this one should live on several lines 

return {
  url: matches[1],
  line: parseInt(matches[2], 10)
};
Attachment #8867343 - Flags: review?(jdescottes)
(In reply to Julian Descottes [:jdescottes] from comment #10)

> Note: if you got the debugger tab, and then go back to the inspector, then
> it works.
> Maybe this is not related to this patch and is just a generic sourcemap
> issue? In this case feel free to ignore it and log it as a follow up. 

This sounds like bug 1364526, maybe; but I think there may also be a race
where the source map service is started too late and somehow misses out on
some sources.  I have a note on my to-do list to look into this, and file a bug
and fix it if needed.
Comment on attachment 8867343 [details]
Bug 1333352 - use client-side source map service in markup view event bubble;

https://reviewboard.mozilla.org/r/138868/#review144612

> is this in order to call the getter for sourceMapURLService on toolbox and make sure it's not optimized out? 
> 
> (can getters actually be optimized out?)
> 
> In any case can you add a comment explaining why this is needed ?

I just thought this was clearer, but no worries, I've removed "ignore" and added a comment instead.
Comment on attachment 8867343 [details]
Bug 1333352 - use client-side source map service in markup view event bubble;

https://reviewboard.mozilla.org/r/138868/#review144612

> I find this fragile sorry.
> 
> I am thinking that if we have to update the DOM, we might as well store the URI here. Well it's already in the textContent and the title, but maybe add it as data-attribute that we could read from debug-clicked? 
> 
> Not blocking the review on this, though.

I don't mind changing this, but I want to understand exactly which part it is you'd like changed.

Before my patch the code already had this `_eventEditors` weak map that uses some DOM element as a key and the state object as a value.  My patch rearranges the code a bit to create the state object earlier, so it can be in scope of the source-mapping promise.  Is it the weak map you dislike (that's the part I am confused by, so that's why I ask).  One other approach I considered was to hoist the creation of the `content` element up higher in the function, and then refer to that from the promise ... is that what you were thinking?
One question for you in comment #13.
Flags: needinfo?(jdescottes)
(In reply to Tom Tromey :tromey from comment #13)
> Comment on attachment 8867343 [details]
> Bug 1333352 - use client-side source map service in markup view event bubble;
> 
> https://reviewboard.mozilla.org/r/138868/#review144612
> 
> > I find this fragile sorry.
> > 
> > I am thinking that if we have to update the DOM, we might as well store the URI here. Well it's already in the textContent and the title, but maybe add it as data-attribute that we could read from debug-clicked? 
> > 
> > Not blocking the review on this, though.
> 
> I don't mind changing this, but I want to understand exactly which part it
> is you'd like changed.
> 
> Before my patch the code already had this `_eventEditors` weak map that uses
> some DOM element as a key and the state object as a value.  My patch
> rearranges the code a bit to create the state object earlier, so it can be
> in scope of the source-mapping promise.  Is it the weak map you dislike
> (that's the part I am confused by, so that's why I ask).  

The weak map is OK :) The part I dislike is that we modify the "state" object based on the fact that we know it will get inserted in the weakmap, and that nothing will modify this weakmap entry afterwards. We trust that this state object, locally created a few lines above, will remain the source of truth. In my mind this a lot to expect from this object without enforcing it.
 
> One other approach
> I considered was to hoist the creation of the `content` element up higher in
> the function, and then refer to that from the promise ... is that what you
> were thinking?

If I understand correctly, you suggest to get the appropriate state from the weakmap by using the content. Yes, I think it's better than the current solution, I'm fine with this too!
Flags: needinfo?(jdescottes)
This new version takes the approach used elsewhere in the file, e.g., _headerClicked.
Comment on attachment 8867343 [details]
Bug 1333352 - use client-side source map service in markup view event bubble;

https://reviewboard.mozilla.org/r/138868/#review145102

Looks great, thanks for updating the patch!

::: devtools/client/inspector/markup/test/browser_markup_events_source_map.js:43
(Diff revisions 3 - 4)
>    // sources as they are loaded, avoiding any races.
>    let {toolbox, inspector, testActor} = yield openInspectorForURL(INITIAL_URL);
>  
> -  // Ensure the source map service is operating.
> -  ignore(toolbox.sourceMapURLService);
> +  // Ensure the source map service is operating.  This looks a bit
> +  // funny, but sourceMapURLService is a getter, and we don't need the
> +  // result.

Sorry, I really didn't understand that "I" was supposed to ignore the ignore(), :)
Attachment #8867343 - Flags: review?(jdescottes) → review+
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e82c93c18e8d
use client-side source map service in markup view event bubble; r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/e82c93c18e8d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
https://hg.mozilla.org/projects/cedar/rev/e82c93c18e8d9b2f5b820a672b55ee467f27a985
Bug 1333352 - use client-side source map service in markup view event bubble; r=jdescottes
Depends on: 1368695
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: