JS Source: Native["java/lang/System.arraycopy.(Ljava/lang/Object;ILjava/lang/Object;II)V"] = function () { ... } Profiler Sample Location String: Native["java/lang/System.arraycopy.(Ljava/lang/Object;ILjava/lang/Object;II)V"] (http://localhost:8080/bld/main-all.js:262) functionName is parsed incorrectly as: Native["java/lang/System.arraycopy

This function is also incorrectly tagged as being chrome code. Huh.

Ughghghghhg. So this will still fail probably on brackets in query params in the URL or something, but progress. https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fce94549d29

Ugh, well the problem here is that the engine can name functions with parentheses in them. Skipping over brackets isn't the right solution here. Let me think about what to do here... P.S. The profiler putting the function name and the location in the same string is also super dumb.

It's not a good solution, but it works; what scenario would parentheses exist in a function name outside of square brackets? There is no perfect (or even arguably good) solution for this given the SPS profiler output, but this atleast fixes the function not being labeled as chrome code, and practically speaking don't have more time to give on this

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #4) > It's not a good solution, but it works; what scenario would parentheses > exist in a function name outside of square brackets? There is no perfect (or > even arguably good) solution for this given the SPS profiler output, but > this atleast fixes the function not being labeled as chrome code, and > practically speaking don't have more time to give on this 21:44 < shu> oh maybe it's as simple as the first space from the right 21:44 < shu> if there is a space in the filename, it must be urlencoded 21:45 < shu> wait that doesn't work 21:45 < shu> wait 21:45 < shu> okay that works 21:45 < shu> there are 3 possibilities 21:45 < shu> "name (resource:line)" 21:45 < shu> "resource:line" 21:45 < shu> "resource" 21:46 < shu> for "name (resource:line)", the rightmost space must be the separating space, because spaces in resource are urlencoded 21:46 < shu> for "resource:line" and "resource", there is no rightmost space, so assume rightmost space = 0

Comment on attachment 8652660 [details] [diff] [review] Rework profiler location string parsing to scan from the right. Review of attachment 8652660 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/test/unit/test_frame-utils-01.js @@ +26,5 @@ > + "hello/<.world (http://localhost:8888/:100:50)", > + > + // bug 1197636 > + "Native[\"arraycopy(blah)\"] (http://localhost:8888/profiler.html:4)", > + "Native[\"java/[lang](blah)\"] (http://localhost:8888/:1)", This second line was testing specifically extra brackets in the string, due to the implementation of my patch. So if there is any implementation weirdness you can forsee, maybe a test for that, if not remove this We should add lines here for the 3 types of strings mentioned in frame-utils (not sure if this test already has those or not). Can any of these profiler strings contain column numbers as well?

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7) > Comment on attachment 8652660 [details] [diff] [review] > Rework profiler location string parsing to scan from the right. > > Review of attachment 8652660 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/devtools/performance/test/unit/test_frame-utils-01.js > @@ +26,5 @@ > > + "hello/<.world (http://localhost:8888/:100:50)", > > + > > + // bug 1197636 > > + "Native[\"arraycopy(blah)\"] (http://localhost:8888/profiler.html:4)", > > + "Native[\"java/[lang](blah)\"] (http://localhost:8888/:1)", > > This second line was testing specifically extra brackets in the string, due > to the implementation of my patch. So if there is any implementation > weirdness you can forsee, maybe a test for that, if not remove this > Okay. > Can any of these profiler strings contain column numbers as well? Yes.

Then let's add tests for the 3 scenarios mentioned in frame utils if they're not already covered, plus each one with column numbers then looks good to me