aspnetcore icon indicating copy to clipboard operation
aspnetcore copied to clipboard

[dotnet-sdk-9.0.100-preview.7.24379.15] Fail to load page in nopCommerceNetCore app after retargeting it to .NET 9.0 with error: System.NullReferenceException: Object reference not set to an instance of an object.

Open Junjun-zhao opened this issue 1 year ago • 34 comments

Description

When retarget the 3rd party application to net9.0, build and run with the latest .NET 9 build, it failed to load page with error: System.NullReferenceException: Object reference not set to an instance of an object. When retargeting it to net9.0 using dotnet-sdk-9.0.100-preview.5.24307.3, this issue does not reproduce.

Findings: After debugging into the source code of application, we found the exception is thrown from NUglify nuget package, which is referenced by WebMarkupMin.NUglify nuget package. And if we reference the NUglify and WebMarkupMin.NUglify nuget packages source code into application source code directly, we found the exception is thrown at JSParser.ParseUnaryExpression() method in JSParser.cs file (line 3665) from NUglify package. The expr variable in line 3664 returns a null object, which result to the line 3665 get "Object reference not set to an instance of an object" exception:

expr = ParseUnaryExpression(out dummy, false); //It returns null
ast = new UnaryExpression(exprCtx.CombineWith(expr.Context)) //The null expr causes the error
    {
        Operand = expr,
        OperatorContext = exprCtx,
        OperatorToken = opToken
    };

Reproduction Steps

Please get the repro machine information from Internal bug 1.Open "...\Source\nopCommerce\src\Presentation\Nop.Web\Nop.Web.csproj" with Visual Studio. 2. Retarget Nop.Web to net9.0. <TargetFramework>net9.0</TargetFramework> 3. Build the app with dotnet-sdk-9.0.100-preview.7.24379.15. 4. Run the app. 5. Go to "https://localhost:56111/Admin/Order/List" on browser.(If need login, account and password refer to Internal bug) 6. Click "View" in order list.

Expected behavior

Page loading successfully

Actual behavior

Page loaded failed with errors: System.NullReferenceException: Object reference not set to an instance of an object.

StackTrace:

System.NullReferenceException: Object reference not set to an instance of an object.
at NUglify.JavaScript.JSParser.ParseUnaryExpression(Boolean& isLeftHandSideExpr, Boolean isMinus)
   at NUglify.JavaScript.JSParser.ParseExpressionStatement(Boolean fSourceElement)
   at NUglify.JavaScript.JSParser.ParseStatement(Boolean fSourceElement, Boolean skipImportantComment)
   at NUglify.JavaScript.JSParser.ParseStatements(BlockStatement block)
   at NUglify.JavaScript.JSParser.InternalParse()
   at NUglify.JavaScript.JSParser.Parse(DocumentContext sourceContext)
   at WebMarkupMin.NUglify.NUglifyJsMinifier.Minify(String content, Boolean isInlineCode, Encoding encoding)
   at WebMarkupMin.NUglify.NUglifyJsMinifier.Minify(String content, Boolean isInlineCode)
   at WebMarkupMin.Core.GenericHtmlMinifier.ProcessEmbeddedScriptContent(MarkupParsingContext context, String content, String contentType)
   at WebMarkupMin.Core.GenericHtmlMinifier.EmbeddedCodeHandler(MarkupParsingContext context, String code)
   at WebMarkupMin.Core.Parsers.HtmlParser.ProcessEmbeddedCode()
   at WebMarkupMin.Core.Parsers.HtmlParser.Parse(String content)
   at WebMarkupMin.Core.GenericHtmlMinifier.Minify(String content, String fileContext, Encoding encoding, Boolean generateStatistics)
   at WebMarkupMin.Core.HtmlMinifier.Minify(String content, String fileContext, Encoding encoding, Boolean generateStatistics)
   at WebMarkupMin.AspNetCore8.BodyWrapperStreamBase.<InternalFinishAsync>d__24.MoveNext()
   at WebMarkupMin.AspNetCore8.BodyWrapperStreamWithResponseBodyFeature.<FinishAsync>d__5.MoveNext()
   at WebMarkupMin.AspNetCore8.WebMarkupMinMiddleware.<InvokeCore>d__2.MoveNext()
   at WebMarkupMin.AspNetCore8.WebMarkupMinMiddleware.<InvokeCore>d__2.MoveNext()
   at Microsoft.AspNetCore.Localization.RequestLocalizationMiddleware.<Invoke>d__5.MoveNext()
   at Microsoft.AspNetCore.Session.SessionMiddleware.<Invoke>d__8.MoveNext()
   at Microsoft.AspNetCore.Session.SessionMiddleware.<Invoke>d__8.MoveNext()
   at Nop.Services.Installation.InstallUrlMiddleware.<InvokeAsync>d__2.MoveNext() in C:\Users\test01\Desktop\Source\nopCommerce\src\Libraries\Nop.Services\Installation\InstallUrlMiddleware.cs:line 50

Regression?

Yes

Verify Scenarios: 1). Windows 10 21H2 AMD64 + default target net8.0 + default dotnet-sdk-8.0.300: Pass 2). Windows 10 21H2 AMD64 + default target net8.0 + dotnet-sdk-9.0.100-preview.7.24379.15: Pass 3). Windows 10 21H2 AMD64 + target net9.0 + dotnet-sdk-9.0.100-preview.5.24307.3: Pass 4). Windows 10 21H2 AMD64 + target net9.0 + dotnet-sdk-9.0.100-preview.6.24328.19: Fail 5). Windows 10 21H2 AMD64 + target net9.0 + dotnet-sdk-9.0.100-preview.7.24375.12: Fail 6). Windows 10 21H2 AMD64 + target net9.0 + dotnet-sdk-9.0.100-preview.7.24379.15: Fail

Known Workarounds

No response

Configuration

Application Name: nopCommerceNetCore OS:Windows 10 21H2 CPU: X64 .NET Build Number: dotnet-sdk-9.0.100-preview.7.24379.15 App Source or Repro machine checking at : https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2187455 GitHub Link: https://github.com/nopSolutions/nopCommerce

Dotnet Info:

.NET SDK:
 Version:           9.0.100-preview.7.24379.15
 Commit:            812e813ae9
 Workload version:  9.0.100-manifests.28aae763
 MSBuild version:   17.12.0-preview-24374-02+48e81c6f1

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19045
 OS Platform: Windows
 RID:         win-x64
 Base Path:   C:\Program Files\dotnet\sdk\9.0.100-preview.7.24379.15\

.NET workloads installed:
Configured to use loose manifests when installing new manifests.
There are no installed workloads to display.

Host:
  Version:      9.0.0-preview.7.24376.15
  Architecture: x64
  Commit:       static

.NET SDKs installed:
  3.0.103 [C:\Program Files\dotnet\sdk]
  5.0.100 [C:\Program Files\dotnet\sdk]
  6.0.100 [C:\Program Files\dotnet\sdk]
  8.0.400-preview.0.24324.5 [C:\Program Files\dotnet\sdk]
  9.0.100-preview.7.24379.15 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 3.0.3 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 9.0.0-preview.7.24379.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 3.0.3 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 9.0.0-preview.7.24376.15 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.0.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 8.0.5 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
  Microsoft.WindowsDesktop.App 9.0.0-preview.7.24379.3 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other information

@dotnet-actwx-bot @dotnet/compat

Junjun-zhao avatar Aug 05 '24 12:08 Junjun-zhao

@jeffschwMSFT This issue was found during source compat test. After retargeting to net9.0 with the latest build, fail to load page. It passed with dotnet-sdk-9.0.100-preview.5.24307.3.
Could you please help check this issue and confirm if this is a blocker for .NET 9 preview7? Thanks.

Junjun-zhao avatar Aug 05 '24 12:08 Junjun-zhao

NUglify was one of the places where we found quite a few opportunities for object stack allocation. However those changes were not in preview 6, and per above this fails on preview 6 too.

I can take a look.

AndyAyersMS avatar Aug 05 '24 16:08 AndyAyersMS

The crash happens in a debugger-launched instance, so it's very unlikely to be anything related to codegen.

As best I can tell the minifier is working on the following text and seems to get confused by the content

<script id="download1532333276-qq-template" type="text/javascript">
                    <div class="qq-uploader-selector qq-uploader">
                        <div class="qq-upload-drop-area-selector qq-upload-drop-area" qq-hide-dropzone>
                            <span>Drop files here to upload</span>

Trying to parse the <div as javascript.

Attached an attempt to repro but it is not failing for me, so it is likely missing something important.

ReproAttempt.zip

Better repro attempt. This fails the same way on .NET 8 and .NET 9 and fails like the app does:

ImprovedReproAttempt.zip

AndyAyersMS avatar Aug 06 '24 20:08 AndyAyersMS

Need to figure out who can look at this further.

AndyAyersMS avatar Aug 06 '24 21:08 AndyAyersMS

Minimal repro:

using NUglify.JavaScript;

var parser = new JSParser();
var result = parser.Parse("...");

Console.WriteLine(result);

There is a bug in the library, at least. Reproduced on 8.0.303 and 9.0.100-rc.1.24406.16

Issue fixed with a null check in JSParser.cs

if (expr != null)
{
    ast = new UnaryExpression(exprCtx.CombineWith(expr.Context))
    {
        Operand = expr,
        OperatorContext = exprCtx,
        OperatorToken = opToken
    };
}

sebastienros avatar Aug 07 '24 00:08 sebastienros

@Junjun-zhao can you confirm that this app still passes with Preview 5? I'm concerned this investigation is getting side-tracked on the specifics of the crash and missing the behavior difference.

@sebastienros This is an app-compat failure. The app is reported to work on Preview 5 and fails starting with Preview 6. It's totally possible that something external to the app changed, which is why I asked Junjun to double-check the status of preview 5. But, any bug in the library is irrelevant WRT the behavioral regression (assuming it still shows as a regression). We may decide that whatever change in behavior is by design, but we can't do that until we know what that behavior is.

marklio avatar Aug 07 '24 18:08 marklio

I can also take another look, if @Junjun-zhao can't get to this soonish. Was going to try running on .NET 8 to confirm or refute my guess that the HTML to be minified is different.

AndyAyersMS avatar Aug 07 '24 20:08 AndyAyersMS

When built/hosted on .NET 8, the HTML is different, in particular the script type:

<script id="download2074740237-qq-template" type="text/template">
                    <div class="qq-uploader-selector qq-uploader">
                        <div class="qq-upload-drop-area-selector qq-upload-drop-area" qq-hide-dropzone>
                            <span>Drop files here to upload</span>
                        </div>

and there is no crash.

AndyAyersMS avatar Aug 07 '24 21:08 AndyAyersMS

in particular the script type

That makes more sense. Because it's not JS it doesn't trigger the parser nug with the NRE. So nothing to do with NUglify but still a change in behavior between two dotnet versions which needs to be investigated.

sebastienros avatar Aug 07 '24 21:08 sebastienros

Still not sure how to track this down further. The "qq-template" stuff as best I can tell comes from a javascript file upload library FineUploader which got replaced in newer versions nop commerce.

Eg from src/Presentation/Nop.Web/Areas/Admin/Views/Order/_ProductAddAttributes.cshtml

                                        @*fine uploader container*@
                                        <div id="@(controlId)uploader"></div>
                                        @*fine uploader template (keep it synchronized to \Content\fineuploader\templates\default.html)*@
                                        <script asp-location="Footer" type="text/template" id="@(controlId)-qq-template">
                                            <div class="qq-uploader-selector qq-uploader">
                                                <div class="qq-upload-drop-area-selector qq-upload-drop-area" qq-hide-dropzone>

AndyAyersMS avatar Aug 07 '24 22:08 AndyAyersMS

In NopScriptTagHelpers.cs there is this bit of code

    public override async Task ProcessAsync(TagHelperContext context, TagHelperOutput output)
    {
        ArgumentNullException.ThrowIfNull(context);

        ArgumentNullException.ThrowIfNull(output);

        if (_webHelper.IsAjaxRequest(ViewContext.HttpContext?.Request))
            return;

        output.TagMode = TagMode.StartTagAndEndTag;

        if (!output.Attributes.ContainsName("type")) // we don't touch other types e.g. text/template
            output.Attributes.SetAttribute("type", MimeTypes.TextJavascript);

        var woConfig = _appSettings.Get<WebOptimizerConfig>();

so if for some reason ContainsName returned false unexpectedly that could explain the bug. Here output is of type Microsoft.AspNetCore.Razor.TagHelpers.TagHelperOutput

And I can see it get hit in the .NET 8 run with an attribute "type" that is "text/template". Let me see what happens with 9.0...

AndyAyersMS avatar Aug 07 '24 22:08 AndyAyersMS

That seems to be the next piece of the puzzle, there is no "type" attribute in the .NET 9 run, so the code above adds an attribute type="text/javascript" and things go downhill from there.

image

Suggests perhaps it is an issue in ...? Razor? Not sure.

AndyAyersMS avatar Aug 07 '24 23:08 AndyAyersMS

@MackinnonBuck do you have any thoughts about this?

sebastienros avatar Aug 07 '24 23:08 sebastienros

Per @javiercn: seems like it is likely an instance of https://github.com/dotnet/aspnetcore/issues/57138 which was fixed recently by https://github.com/dotnet/aspnetcore/pull/57170

AndyAyersMS avatar Aug 08 '24 00:08 AndyAyersMS

@marklio @AndyAyersMS Yes. This issue doesn't reproduce with Preview 5 for this app. 1). Windows 10 21H2 AMD64 + target net9.0 + dotnet-sdk-9.0.100-preview.5.24307.3: Pass 2). Windows 10 21H2 AMD64 + default target net8.0 + dotnet-sdk-8.0.303: Pass

Junjun-zhao avatar Aug 08 '24 01:08 Junjun-zhao

@AndyAyersMS @javiercn According to https://github.com/dotnet/aspnetcore/issues/57493, it seems an instance of https://github.com/dotnet/aspnetcore/issues/57138. We verified https://github.com/dotnet/aspnetcore/issues/57138 with latest build dotnet-sdk-9.0.100-rc.1.24409.1, it has been fixed. But this issue still reproduce, could you please take a look at this issue again? Thanks

Junjun-zhao avatar Aug 12 '24 08:08 Junjun-zhao

@javiercn can you or somebody on your team take a look? Since this fails when debugging it is unlikely to be something low-level causing the problem.

AndyAyersMS avatar Aug 12 '24 18:08 AndyAyersMS

@mkArtakMSFT should I transfer this one over to dotnet/aspnetcore?

I'm happy to help out if needed.

AndyAyersMS avatar Aug 16 '24 22:08 AndyAyersMS

Ping @mkArtakMSFT. Could you please take a look? We think we need to transfer this issue to aspnetcore.

@mkArtakMSFT should I transfer this one over to dotnet/aspnetcore?

I'm happy to help out if needed.

JulieLeeMSFT avatar Aug 23 '24 18:08 JulieLeeMSFT

@adityamandaleeka sorry for cross-nagging you, but since Dan is out... can you help make sure this gets attention?

AndyAyersMS avatar Aug 23 '24 19:08 AndyAyersMS

@AndyAyersMS ACK.

adityamandaleeka avatar Aug 23 '24 22:08 adityamandaleeka

I've moved this to a milestone and put it in our project board. It was incorrectly labeled so we lost track of it.

I've also taken a quick look at the issue, but the Visual Studio instance on the repro machine has an expired license.

javiercn avatar Aug 24 '24 14:08 javiercn

@Junjun-zhao can you make sure visual studio is up to date on the repro machine and usable?

javiercn avatar Aug 24 '24 15:08 javiercn

@Junjun-zhao this appears to be fixed.

image

image

javiercn avatar Aug 24 '24 16:08 javiercn

And this is the same running with RC2 image

image

javiercn avatar Aug 24 '24 17:08 javiercn

Wrong screenshot, but still works image

javiercn avatar Aug 24 '24 17:08 javiercn

Hi @javiercn,

We verified with RC1 and RC2 build, this issue still reproduces. 1). Windows 10 21H2 AMD64 + target net9.0 +dotnet-sdk-9.0.100-preview.7.24407.12: Fail 2). Windows 10 21H2 AMD64 + target net9.0 +dotnet-sdk-9.0.100-rc.1.24422.10: Fail 3). Windows 10 21H2 AMD64 + target net9.0 +dotnet-sdk-9.0.100-rc.2.24425.2: Fail

Here is a video recorded for your reference: The Visual Studio on repro machine has been activated and updated to the latest version. ReproSteps (1)

Junjun-zhao avatar Aug 26 '24 09:08 Junjun-zhao

@AndyAyersMS we took a look.

We can only repro with the exact steps provided here. And when we look at the call stack, the markup throwing the exception is identical between 8.0 and 9.0.

Here is the markup from the callsite:

<!DOCTYPE html>
<html lang="en" dir="ltr">
<head>
    <title>Edit order details / nopCommerce administration</title>
    <meta http-equiv="Content-type" content="text/html;charset=UTF-8" />
    <meta content="width=device-width, initial-scale=1, maximum-scale=1, user-scalable=no" name="viewport">
    



<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Source+Sans+Pro:300,400,600,700,300italic,400italic,600italic" />








    
    



<link rel="stylesheet" type="text/css" href="/css/ay9_xl4b4hochtdfvoclsa.styles.css?v=Da65r2jowUqk07cscI7WfcqWWIQ" />

<script type="text/javascript" src="/lib_npm/jquery/jquery.min.js?v=ETctd0sWu37HRGyGqGIsZu5ju2A"></script>

And as I mentioned, is identical between 8.0 and 9.0.

In addition to that, it seems to be machine/environment specific? I can't reproduce the issue on my own machine.

javiercn avatar Aug 26 '24 18:08 javiercn

Is there any appreciable difference you can spot between the machines where you have a repro and the ones where you don't (different OS/CPU etc)?

Possibly this is a different problem with similar manifestation?

I would appreciate it if you could dig a little deeper on the machines where you have a repro. As this is all debug code it is unlikely an issue in the jit or runtime layers. I think it is more likely something upstack. If you run into a dead-end, then let me know.

AndyAyersMS avatar Aug 26 '24 18:08 AndyAyersMS

@AndyAyersMS I'm trying to dig into this.

Unfortunately, I was trying to get more confirmation, and I suspect the reason I can't reproduce it on my box is that I don't seem to be hitting the same code path.

I've taken a look at the machine and the solution seems to have lots of changes applied to them (in my case I cloned and simply switched .net 8.0 to .net 9.0).

I'm somewhat at a loss here because when I hit

		public MarkupMinificationResult Minify(string content, string fileContext, Encoding encoding,
			bool generateStatistics)
		{
			return _genericHtmlMinifier.Minify(content, fileContext, encoding, generateStatistics);
		}

In the repro machine, the values are exactly the same for .NET 8.0 and .NET 9.0. I don't really know where else to look as from the callstack below, the same function is called with the same parameters. In my book that should produce the same result in the absence of any weird side-effect, which I think would be library specific.

System.NullReferenceException: Object reference not set to an instance of an object.
at NUglify.JavaScript.JSParser.ParseUnaryExpression(Boolean& isLeftHandSideExpr, Boolean isMinus)
   at NUglify.JavaScript.JSParser.ParseExpressionStatement(Boolean fSourceElement)
   at NUglify.JavaScript.JSParser.ParseStatement(Boolean fSourceElement, Boolean skipImportantComment)
   at NUglify.JavaScript.JSParser.ParseStatements(BlockStatement block)
   at NUglify.JavaScript.JSParser.InternalParse()
   at NUglify.JavaScript.JSParser.Parse(DocumentContext sourceContext)
   at WebMarkupMin.NUglify.NUglifyJsMinifier.Minify(String content, Boolean isInlineCode, Encoding encoding)
   at WebMarkupMin.NUglify.NUglifyJsMinifier.Minify(String content, Boolean isInlineCode)
   at WebMarkupMin.Core.GenericHtmlMinifier.ProcessEmbeddedScriptContent(MarkupParsingContext context, String content, String contentType)
   at WebMarkupMin.Core.GenericHtmlMinifier.EmbeddedCodeHandler(MarkupParsingContext context, String code)
   at WebMarkupMin.Core.Parsers.HtmlParser.ProcessEmbeddedCode()
   at WebMarkupMin.Core.Parsers.HtmlParser.Parse(String content)
   at WebMarkupMin.Core.GenericHtmlMinifier.Minify(String content, String fileContext, Encoding encoding, Boolean generateStatistics)
   at WebMarkupMin.Core.HtmlMinifier.Minify(String content, String fileContext, Encoding encoding, Boolean generateStatistics)

@Junjun-zhao I see a lot of files modified on that solution. Can we park this machine and try the scenario from scratch on a clean machine?

Essentially, if I go to the machine and I do git diff I only want to see the change from 8.0 to 9.0 and the change in global.json

javiercn avatar Aug 26 '24 19:08 javiercn