NPE when configuration property missing in SuperDevMode #9806

Activity

1. niloc132 commented on Mar 15, 2023

   

   niloc132

   on Mar 15, 2023

   Member

   More actions

   I've been playing with this, and there are a few other bugs in the implementation that we're stuck with. First off, the <source info not available> comes from the broken ToStringGenerationVisitor#visit(com.google.gwt.dev.jjs.ast.JPermutationDependentValue, com.google.gwt.dev.jjs.ast.Context) implementation, it tries to emit a GWT.getProperty(...) line, but no such method exists (or ever existed) - my hunch is that before it was implemented with System.getProperty, it was going to be GWT.getProperty, and have the same contract - take a string and an optional default.

   So, the question is, what do we want this to do, what was the original intent, and how worried are we about adding new unexpected behavior or breakage?

   UnifyAst/UnifyAstTest

   • UnifyAstTest.testGetProperty_error_undefinedProperty shows that if the property doesnt exist, compile will fail
   • testGetPropertyWithDefault_success_undefined however expands to show that if the property doesn't exist, but a default is specified, the compile should not fail.
   • UnifyAstTest.testGetProperty_error_multivalued shows that if the property exists and is multivalued, even if there is a default, compile will fail.
   • There is no test for a defined config property. The closest we get here is defining a multi value property, so we can fail on it. Only the later work in JPermutationDependentValue handles this case.
   • No selection properties are tested (aside from their absence with no default)

   JPermutationDependentValue/SystemTest

   • SystemTest.testGetProperty() comes with a .gwt.xml file to set a few properties, in EmulSuite.gwt.xml. A single config prop is defined there, with a value set on it.
   • There is a test for a defined configuration property, to make sure we get the right value even if there is a default provided
   • There is a test for non-existent configuration properties, to make sure you can pass in literals or computed properties as defaults.
   • No test exists for defining but not setting a config property

   SystemGetPropertyTest

   • This tests only selection properties, reading values from property providers and from collapsed properties (i.e. runtime lookup)
   • There are no tests for default values, since a selection property cannot be null.

   So it seems that only a non-multi-value config property could be defined but never set - do we want that to behave as though it was set to null? That is,

   • Do we allow null with no default,
   • Or do we require either that a value be set in the config property or a default expression (including null) passed to System.getProperty (validate in UnifyAst)?
2. jnehlmeier commented on Mar 15, 2023

   

   jnehlmeier

   on Mar 15, 2023

   Member

   More actions

   @niloc132 As far as I can see there are non-multi-valued configuration properties in GWT that are never set. However in these cases the code always provides a default when asking for that possibly not initialized configuration property.

   Some examples are:

   • compiler.emulatedStack.recordLineNumbers
   • compiler.splitpoint.leftovermerge.size
   • devModeRedirectEnabled
   • devModeUrlWhitelistRegexp

   For gwt-safehtml the property in question has been newly introduced but the code fails to provide a default value when checking the property value. So IMHO it is a bug in gwt-safehtml and we should not change anything in GWT compiler except providing a good error message and adding a test for that scenario.

   Of course one could argue that the code in GWT SDK always provides a default value because it knows it would fail otherwise. I am pretty sure some code in GWT SDK would have looked the same as in gwt-safehtml if it would not fail (gwt-safehtml just does a getProperty(..) != null check). But I feel like we should not be in a situation where the code does not care about the actual value, even if the property is just an on/off toggle. That is why I would vote for keeping the current behavior and fix gwt-safehtml instead.
3. jnehlmeier commented on Mar 15, 2023

   

   jnehlmeier

   on Mar 15, 2023

   Member

   More actions

   For gwt-safehtml the property in question has been newly introduced but the code fails to provide a default value when checking the property value.

   Meh...I used to org.gwtproject... prefixed name for the search. The property isn't new and GWT SDK should have the same issue as it also just does a != null, see

   gwt/user/src/com/google/gwt/safecss/shared/SafeStylesHostedModeUtils.java

   Line 298 in 7c96744

   forceCheck = System.getProperty(FORCE_CHECK_VALID_STYLES) != null;

   The reason why it works in GWT SDK but not in gwt-safehtml is that GWT SDK provides a super-source version that does not check the property at all. See:

   gwt/user/super/com/google/gwt/safecss/super/com/google/gwt/safecss/shared/SafeStylesHostedModeUtils.java

   Line 27 in 7c96744

   public static final String FORCE_CHECK_VALID_STYLES = null;

   gwt-safehtml does not have that super-sourced version.
4. niloc132 commented on Mar 15, 2023

   

   niloc132

   on Mar 15, 2023

   Member

   More actions

   I certainly agree that the property isn't very helpful if it is not set, and definitely agree we should fix gwt-safehtml regardless of what we do here.

   However, I think it is also reasonable to expand the behavior here a bit. Specific reasons:

   • The system already isn't very well fleshed out (can't tostring a System.getProperty AST node without an NPE).
   • J2CL+Closure also uses System.getProperty, and doesn't require a value be specified (note however that if you don't have a @define, it seems you can't have a --define, but you will still get null or be able to use your default value, and the @define does not have to be in the same module...).
   • Java doesn't require a value to be defined, and avoiding super-source, just sharing "clearly i didnt set this, just give me null" logic can be surprising for it not to work.

   Add-on to respond to your second reply: the method in the org.gwtproject.safehtml repo isn't actually called, but GWT still tries to "fix it" in SDM when it rewrites the magic method without any dead code elimination. Not using the code is no protection against the code violating the unwritten rules (so let's make the rules make sense?).

   So I'm not just asking "what was this meant to be, in the eyes of the original authors", but "what should it be, to make sense to future users". I think a reasonable case can be made for "it doesnt make sense as is, fails very poorly, and might not have been fully thought out, so maybe we can make some guarantees a bit stronger". If there is a compelling reason I'm not seeing for supporting returning null when no value was defined and no default provided (as j2cl and the jvm will do), then I can remove that extra bit from my patch and still fix the rest of this.
5. zbynek commented on Mar 15, 2023

   

   zbynek

   on Mar 15, 2023

   Last edited by zbynek

   CollaboratorAuthor

   More actions

   can remove that extra bit from my patch and still fix the rest of this.

   @niloc132 do you have a patch for this? Should I close the draft PR?

   Proposal with enforcing a default sounds good.
6. niloc132 commented on Mar 15, 2023

   

   niloc132

   on Mar 15, 2023

   Member

   More actions

   @zbynek yes go ahead and close yours, I'll build off it and propose something new. Want to look at the gwt-safehtml side of things instead?
7. niloc132 commented on Mar 25, 2023

   

   niloc132

   on Mar 25, 2023

   Last edited by niloc132

   Member

   More actions

   @zbynek I'm trying to reproduce your initial example with stock GWT to ensure that the fix we've made makes sense, because there are a few parts that don't make sense.

   It turns out that the ResolvePermutationDependentValues.ValueReplacer extends JVisitor, and calls ValueReplacer.accept(program), which has a try/catch in it (and not just for RuntimeException, but for Throwable itself):
   

   gwt/dev/core/src/com/google/gwt/dev/jjs/ast/JVisitor.java

   Lines 121 to 132 in 7c96744

   	public JNode accept(JNode node) {
   	return accept(node, false);
   	}
   	public JNode accept(JNode node, boolean allowRemove) {
   	try {
   	node.traverse(this, UNMODIFIABLE_CONTEXT);
   	return node;
   	} catch (Throwable e) {
   	throw translateException(node, e);
   	}
   	}

   I tried to modify the Hello sample to reproduce this, by just adding a configuration property to the .gwt.xml without a value, and using it with System.getProperty. Here's the full error I got - note that it looks like your error almost exactly, until the NPE's stack trace ends:

        [java] com.google.gwt.dev.jjs.InternalCompilerException: Unexpected error during visit.
        [java]     at com.google.gwt.dev.jjs.ast.JVisitor.translateException(JVisitor.java:111)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.accept(JModVisitor.java:276)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.accept(JModVisitor.java:265)
        [java]     at com.google.gwt.dev.jjs.ast.JVisitor.accept(JVisitor.java:118)
        [java]     at com.google.gwt.dev.jjs.ast.JBinaryOperation.traverse(JBinaryOperation.java:89)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.traverse(JModVisitor.java:361)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.acceptImmutable(JModVisitor.java:305)
        [java]     at com.google.gwt.dev.jjs.ast.JMethodCall.visitChildren(JMethodCall.java:275)
        [java]     at com.google.gwt.dev.jjs.ast.JNewInstance.traverse(JNewInstance.java:74)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.traverse(JModVisitor.java:361)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.accept(JModVisitor.java:273)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.accept(JModVisitor.java:265)
        [java]     at com.google.gwt.dev.jjs.ast.JVisitor.accept(JVisitor.java:118)
        [java]     at com.google.gwt.dev.jjs.ast.JDeclarationStatement.traverse(JDeclarationStatement.java:49)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor$ListContext.traverse(JModVisitor.java:88)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.acceptWithInsertRemove(JModVisitor.java:331)
        [java]     at com.google.gwt.dev.jjs.ast.JBlock.traverse(JBlock.java:94)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.traverse(JModVisitor.java:361)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.accept(JModVisitor.java:273)
        [java]     at com.google.gwt.dev.jjs.ast.JVisitor.accept(JVisitor.java:139)
        [java]     at com.google.gwt.dev.jjs.ast.JVisitor.accept(JVisitor.java:135)
        [java]     at com.google.gwt.dev.jjs.ast.JMethodBody.traverse(JMethodBody.java:83)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.traverse(JModVisitor.java:361)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.accept(JModVisitor.java:273)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.accept(JModVisitor.java:265)
        [java]     at com.google.gwt.dev.jjs.ast.JMethod.visitChildren(JMethod.java:786)
        [java]     at com.google.gwt.dev.jjs.ast.JMethod.traverse(JMethod.java:778)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor$ListContextImmutable.traverse(JModVisitor.java:169)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.acceptWithInsertRemoveImmutable(JModVisitor.java:336)
        [java]     at com.google.gwt.dev.jjs.ast.JClassType.traverse(JClassType.java:147)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.traverse(JModVisitor.java:361)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.accept(JModVisitor.java:273)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.accept(JModVisitor.java:265)
        [java]     at com.google.gwt.dev.jjs.ast.JProgram.visitModuleTypes(JProgram.java:1284)
        [java]     at com.google.gwt.dev.jjs.ast.JProgram.traverse(JProgram.java:1249)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.traverse(JModVisitor.java:361)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.accept(JModVisitor.java:273)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.accept(JModVisitor.java:265)
        [java]     at com.google.gwt.dev.jjs.impl.ResolvePermutationDependentValues.execImpl(ResolvePermutationDependentValues.java:168)
        [java]     at com.google.gwt.dev.jjs.impl.ResolvePermutationDependentValues.exec(ResolvePermutationDependentValues.java:118)
        [java]     at com.google.gwt.dev.jjs.JavaToJavaScriptCompiler.compilePermutation(JavaToJavaScriptCompiler.java:333)
        [java]     at com.google.gwt.dev.jjs.JavaToJavaScriptCompiler.compilePermutation(JavaToJavaScriptCompiler.java:272)
        [java]     at com.google.gwt.dev.CompilePerms.compile(CompilePerms.java:198)
        [java]     at com.google.gwt.dev.ThreadedPermutationWorkerFactory$ThreadedPermutationWorker.compile(ThreadedPermutationWorkerFactory.java:50)
        [java]     at com.google.gwt.dev.PermutationWorkerFactory$Manager$WorkerThread.run(PermutationWorkerFactory.java:74)
        [java]     at java.lang.Thread.run(Thread.java:750)
        [java] Caused by: java.lang.NullPointerException
        [java]     at com.google.gwt.thirdparty.guava.common.base.Preconditions.checkNotNull(Preconditions.java:212)
        [java]     at com.google.gwt.thirdparty.guava.common.base.Joiner.toString(Joiner.java:447)
        [java]     at com.google.gwt.thirdparty.guava.common.base.Joiner.appendTo(Joiner.java:110)
        [java]     at com.google.gwt.thirdparty.guava.common.base.Joiner.appendTo(Joiner.java:154)
        [java]     at com.google.gwt.thirdparty.guava.common.base.Joiner.join(Joiner.java:197)
        [java]     at com.google.gwt.thirdparty.guava.common.base.Joiner.join(Joiner.java:186)
        [java]     at com.google.gwt.dev.jjs.impl.ResolvePermutationDependentValues$ValueReplacer.propertyValueExpression(ResolvePermutationDependentValues.java:84)
        [java]     at com.google.gwt.dev.jjs.impl.ResolvePermutationDependentValues$ValueReplacer.endVisit(ResolvePermutationDependentValues.java:74)
        [java]     at com.google.gwt.dev.jjs.ast.JPermutationDependentValue.traverse(JPermutationDependentValue.java:112)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.traverse(JModVisitor.java:361)
        [java]     at com.google.gwt.dev.jjs.ast.JModVisitor.accept(JModVisitor.java:273)
        [java]     ... 44 more
        [java]                [ERROR] at Hello.java(41): <source info not available>
        [java]                   com.google.gwt.dev.jjs.ast.JPermutationDependentValue
        [java]                [ERROR] at Hello.java(41): <source info not available>
        [java]                   com.google.gwt.dev.jjs.ast.JBinaryOperation
        [java]                [ERROR] at Hello.java(41): <source info not available>
        [java]                   com.google.gwt.dev.jjs.ast.JNewInstance
        [java]                [ERROR] at Hello.java(41): <source info not available>
        [java]                   com.google.gwt.dev.jjs.ast.JDeclarationStatement
        [java]                [ERROR] at Hello.java(40): <source info not available>
        [java]                   com.google.gwt.dev.jjs.ast.JBlock
        [java]                [ERROR] at Hello.java(40): <source info not available>
        [java]                   com.google.gwt.dev.jjs.ast.JMethodBody
        [java]                [ERROR] at Hello.java(40): com.google.gwt.sample.hello.client.Hello.onModuleLoad()V
        [java]                   com.google.gwt.dev.jjs.ast.JMethod
        [java]                [ERROR] at Hello.java(23): com.google.gwt.sample.hello.client.Hello (extends Object implements EntryPoint)
        [java]                   com.google.gwt.dev.jjs.ast.JClassType
        [java]                [ERROR] at Unknown(0): <JProgram>
        [java]                   com.google.gwt.dev.jjs.ast.JProgram
        [java]             [ERROR] Unrecoverable exception, shutting down
        [java] com.google.gwt.core.ext.UnableToCompleteException: (see previous log entries)
        [java]     at com.google.gwt.dev.javac.CompilationProblemReporter.logAndTranslateException(CompilationProblemReporter.java:106)
        [java]     at com.google.gwt.dev.jjs.JavaToJavaScriptCompiler.compilePermutation(JavaToJavaScriptCompiler.java:459)
        [java]     at com.google.gwt.dev.jjs.JavaToJavaScriptCompiler.compilePermutation(JavaToJavaScriptCompiler.java:272)
        [java]     at com.google.gwt.dev.CompilePerms.compile(CompilePerms.java:198)
        [java]     at com.google.gwt.dev.ThreadedPermutationWorkerFactory$ThreadedPermutationWorker.compile(ThreadedPermutationWorkerFactory.java:50)
        [java]     at com.google.gwt.dev.PermutationWorkerFactory$Manager$WorkerThread.run(PermutationWorkerFactory.java:74)
        [java]     at java.lang.Thread.run(Thread.java:750)
        [java]             [ERROR] Not all permutation were compiled , completed (0/1)
        [java]       [WARN] recompile failed
        [java]       [WARN] continuing to serve previous version
   

   At the very least I would expect to see the Not all permutation were compiled , completed (0/1) sub-message, and am surprised that it is missing - or is it possible that the <source info not available> were useless enough that you left the rest out? If so, simply fixing the ToStringGenerationVisitor will correct that.

   Did your added try/catch fix the issue in your own app? And did your log end as in the ticket, or did it have the extra (and useless) <source info not available> on it?
8. zbynek commented on Mar 25, 2023

   

   zbynek

   on Mar 25, 2023

   CollaboratorAuthor

   More actions

   yes, equivalent of

   [java]                [ERROR] at Hello.java(41): <source info not available>
        [java]                   com.google.gwt.dev.jjs.ast.JPermutationDependentValue
        [java]                [ERROR] at Hello.java(41): <source info not available>
        [java]                   com.google.gwt.dev.jjs.ast.JBinaryOperation
        [java]                [ERROR] at Hello.java(41): <source info not available>
        [java]                   com.google.gwt.dev.jjs.ast.JNewInstance
        [java]                [ERROR] at Hello.java(41): <source info not available>
        [java]                   com.google.gwt.dev.jjs.ast.JDeclarationStatement
        [java]                [ERROR] at Hello.java(40): <source info not available>
        [java]                   com.google.gwt.dev.jjs.ast.JBlock
        [java]                [ERROR] at Hello.java(40): <source info not available>
        [java]                   com.google.gwt.dev.jjs.ast.JMethodBody
        [java]                [ERROR] at Hello.java(40): com.google.gwt.sample.hello.client.Hello.onModuleLoad()V
        [java]                   com.google.gwt.dev.jjs.ast.JMethod
        [java]                [ERROR] at Hello.java(23): com.google.gwt.sample.hello.client.Hello (extends Object implements EntryPoint)
        [java]                   com.google.gwt.dev.jjs.ast.JClassType
        [java]                [ERROR] at Unknown(0): <JProgram>
   

   was there in my case before the fix. With the try-catch I was able to get from

   com.google.gwt.dev.jjs.InternalCompilerException: Unexpected error during visit.

   to

   com.google.gwt.dev.jjs.InternalCompilerException: Problem replacing 'org.gwtproject.safecss.ForceCheckValidStyles'
9. niloc132

   closed this as completedin #9812on Apr 11, 2023
10. niloc132

    added a commit that references this issue on Apr 11, 2023

    Fix error reporting for System.getProperty magic method nodes (#9812)

    ...

    This issue will close once commit 65a0674 is merged into the 'main' branch.

    65a0674
11. zbynek commented on Feb 5, 2025

    

    zbynek

    on Feb 5, 2025

    CollaboratorAuthor

    More actions

    @jnehlmeier

    So IMHO it is a bug in gwt-safehtml and we should not change anything in GWT compiler except providing a good error message and adding a test for that scenario.

    Would it be acceptable for you if the behavior changed and System.getProperty() just returned null with a warning? (see #10013 for more context)
12. jnehlmeier commented on Feb 12, 2025

    

    jnehlmeier

    on Feb 12, 2025

    Member

    More actions

    @zbynek Yes. I am generally fine with changes that better match the behavior described in the JRE JavaDoc. It results in less surprises. In plain Java the method returns null if it cannot find the property and no default is given, so GWT should do the same. The only difference with GWT is that GWT has different ways of providing a property value.
13. zbynek commented on Feb 12, 2025

    

    zbynek

    on Feb 12, 2025

    CollaboratorAuthor

    More actions

    Thanks!