Show release notes on the detail view

RESOLVED FIXED in Firefox 68

Status

()

defect
P1
normal
RESOLVED FIXED
2 months ago
14 days ago

People

(Reporter: mstriemer, Assigned: mstriemer)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 months ago

When an extension has an update you should be able to view its release notes in the tabbed sections on its detail page.

Assignee

Updated

2 months ago
Assignee: nobody → mstriemer
Attachment #9063646 - Attachment description: Bug 1547835 - Release notes → Bug 1547835 - Show release notes on HTML about:addons details

Comment 2

Last month
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0bc1bd7285c1
Show release notes on HTML about:addons details r=aswan,flod,rpl,kmag
Assignee

Comment 4

Last month

Emilio, this is failing on an assertion when setting the slot for an element. I commented out this line [1] and the error goes away.

This is the assertion that's causing the error [2].

Does this seem like a bug? I can't quite grok that c++ code.

 0:23.98 GECKO(43230) Assertion failure: nsContentUtils::ContentIsFlattenedTreeDescendantOf( leaf, aContentRemoved) || leaf->SubtreeRoot()->HasFlag(NODE_IS_ANONYMOUS_ROOT), at /builds/worker/workspace/build/src/dom/events/EventStateManager.cpp:5413
 0:24.60 GECKO(43230) #01: 0x02e84282 (in XUL) + 194
 0:24.60 GECKO(43230) #02: 0x0406e804 (in XUL) + 100
 0:24.61 GECKO(43230) #03: 0x019d160f (in XUL) + 303
 0:24.61 GECKO(43230) #04: 0x019d1363 (in XUL) + 291
 0:24.61 GECKO(43230) #05: 0x01712e61 (in XUL) + 929
 0:24.61 GECKO(43230) #06: 0x018a9710 (in XUL) + 16
 0:24.61 GECKO(43230) #07: 0x01dadad9 (in XUL) + 537
 0:24.61 GECKO(43230) #08: 0x02b6e2e4 (in XUL) + 612
 0:24.61 GECKO(43230) #09: 0x05763199 (in XUL) + 297
 0:24.61 GECKO(43230) #10: 0x05762ae7 (in XUL) + 1095
 0:24.61 GECKO(43230) #11: 0x05764aa5 (in XUL) + 213
 0:24.61 GECKO(43230) #12: 0x05a5bb76 (in XUL) + 630
 0:24.61 GECKO(43230) #13: 0x05a5ac95 (in XUL) + 709
 0:24.61 GECKO(43230) #14: 0x057561ce (in XUL) + 30846
 0:24.61 GECKO(43230) #15: 0x0574e694 (in XUL) + 612
 0:24.61 GECKO(43230) #16: 0x05762a63 (in XUL) + 963
 0:24.61 GECKO(43230) #17: 0x0576f514 (in XUL) + 1588
 0:24.61 GECKO(43230) #18: 0x05757ca4 (in XUL) + 37716
 0:24.61 GECKO(43230) #19: 0x0574e694 (in XUL) + 612
 0:24.62 GECKO(43230) #20: 0x05762a63 (in XUL) + 963
 0:24.62 GECKO(43230) #21: 0x05763a5d (in XUL) + 45
 0:24.62 GECKO(43230) #22: 0x059db909 (in XUL) + 937
 0:24.62 GECKO(43230) #23: 0x05763199 (in XUL) + 297
 0:24.62 GECKO(43230) #24: 0x05762ae7 (in XUL) + 1095
 0:24.62 GECKO(43230) #25: 0x05758aca (in XUL) + 41338
 0:24.62 GECKO(43230) #26: 0x0574e694 (in XUL) + 612
 0:24.62 GECKO(43230) #27: 0x05762a63 (in XUL) + 963
 0:24.62 GECKO(43230) #28: 0x05763a5d (in XUL) + 45
 0:24.62 GECKO(43230) #29: 0x059db909 (in XUL) + 937
 0:24.62 GECKO(43230) #30: 0x05763199 (in XUL) + 297
 0:24.62 GECKO(43230) #31: 0x05762ae7 (in XUL) + 1095
 0:24.62 GECKO(43230) #32: 0x05758aca (in XUL) + 41338
 0:24.62 GECKO(43230) #33: 0x0574e694 (in XUL) + 612
 0:24.62 GECKO(43230) #34: 0x05762a63 (in XUL) + 963
 0:24.62 GECKO(43230) #35: 0x0576f514 (in XUL) + 1588
 0:24.62 GECKO(43230) #36: 0x05757ca4 (in XUL) + 37716
 0:24.62 GECKO(43230) #37: 0x0574e694 (in XUL) + 612
 0:24.62 GECKO(43230) #38: 0x05762a63 (in XUL) + 963
 0:24.62 GECKO(43230) #39: 0x05763a5d (in XUL) + 45
 0:24.62 GECKO(43230) #40: 0x05acb048 (in XUL) + 216
 0:24.62 GECKO(43230) #41: 0x060848ef (in XUL) + 239

For background this component works like a deck and has two children in this patch. One is a <section> and one is a custom element. Only one element should be visible, by setting its slot to "selected". When setting the slot to "selected" this error is cropping up. It's called from an attributeChangedCallback which is triggered from a click handler in another custom element.

[1] https://hg.mozilla.org/integration/autoland/rev/9053b1f316ff269aba9b7b4bcbea0b97f9ca733c#l2.218
[2] https://searchfox.org/mozilla-central/rev/cc280c4be94ff8cf64a27cc9b3d6831ffa49fa45/dom/events/EventStateManager.cpp#5413

Flags: needinfo?(emilio)

Well, that stack isn't particularly helpful, but yeah, it is a bug. Is there any chance you could find a reduced-ish test-case and file a bug so I can look into it?

Flags: needinfo?(emilio)
Assignee

Updated

Last month
See Also: → 1551621

It isn't a reduced test-case yet, but the following C++ call stack looks a bit more detailed and it may be helpful (Coming from one of the failures that triggered the backout: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=246187435&repo=autoland&lineNumber=45707):

Assertion failure: nsContentUtils::ContentIsFlattenedTreeDescendantOf( leaf, aContentRemoved) || leaf->SubtreeRoot()->HasFlag(NODE_IS_ANONYMOUS_ROOT), at z:/build/build/src/dom/events/EventStateManager.cpp:5413
#01: mozilla::PresShell::ContentRemoved(nsIContent *,nsIContent *) [layout/base/PresShell.cpp:4388]
#02: nsNodeUtils::ContentRemoved(nsINode *,nsIContent *,nsIContent *) [dom/base/nsNodeUtils.cpp:208]
#03: nsINode::RemoveChildNode(nsIContent *,bool) [dom/base/nsINode.cpp:1785]
#04: nsContentUtils::SetNodeTextContent(nsIContent *,nsTSubstring<char16_t> const &,bool) [dom/base/nsContentUtils.cpp:4846]
#05: mozilla::dom::FragmentOrElement::SetTextContentInternal(nsTSubstring<char16_t> const &,nsIPrincipal *,mozilla::ErrorResult &) [dom/base/FragmentOrElement.cpp:1136]
#06: static bool mozilla::dom::Node_Binding::set_textContent(struct JSContext *, class JS::Handle<JSObject *>, class nsINode *, class JSJitSetterCallArgs) [s3:gecko-generated-sources:1800fd7fdc3d7796e4be3893aeed339d7f69e2314f9f9fcd657a5207f7402a46547fca4b56c399c63b820e9ac2d1bb4a961b543869a38b3b7a77dc525b8a98c7/dom/bindings/NodeBinding.cpp::867]
#07: mozilla::dom::binding_detail::GenericSetter<mozilla::dom::binding_detail::NormalThisPolicy>(JSContext *,unsigned int,JS::Value *) [dom/bindings/BindingUtils.cpp:3118]
#08: CallJSNative(JSContext *,bool (*)(JSContext *,unsigned int,JS::Value *),JS::CallArgs const &) [js/src/vm/Interpreter.cpp:443]
#09: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:535]
#10: static bool InternalCall(struct JSContext *, const class js::AnyInvokeArgs & const) [js/src/vm/Interpreter.cpp:590]
#11: js::CallSetter(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,JS::Handle<JS::Value>) [js/src/vm/Interpreter.cpp:744]
#12: static bool SetExistingProperty(struct JSContext *, class JS::Handle<JS::PropertyKey>, class JS::Handle<JS::Value>, class JS::Handle<JS::Value>, class JS::Handle<js::NativeObject *>, class JS::Handle<JS::PropertyResult>, class JS::ObjectOpResult & const) [js/src/vm/NativeObject.cpp:2879]
#13: bool js::NativeSetProperty<js::Qualified>(struct JSContext *, class JS::Handle<js::NativeObject *>, class JS::Handle<JS::PropertyKey>, class JS::Handle<JS::Value>, class JS::Handle<JS::Value>, class JS::ObjectOpResult & const) [js/src/vm/NativeObject.cpp:2908]
#14: static bool Interpret(struct JSContext *, class js::RunState & const) [js/src/vm/Interpreter.cpp:2847]
#15: js::RunScript(JSContext *,js::RunState &) [js/src/vm/Interpreter.cpp:423]
#16: js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct) [js/src/vm/Interpreter.cpp:563]
#17: static bool InternalCall(struct JSContext *, const class js::AnyInvokeArgs & const) [js/src/vm/Interpreter.cpp:590]
#18: js::Call(JSContext *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,js::AnyInvokeArgs const &,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:606]
#19: js::SpreadCallOperation(JSContext *,JS::Handle<JSScript *>,unsigned char *,JS::Handle<JS::Value>,JS::Handle<JS::Value>,JS::Handle<JS::Value>,JS::Handle<JS::Value>,JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:0]
#20: js::jit::DoSpreadCallFallback(JSContext *,js::jit::BaselineFrame *,js::jit::ICCall_Fallback *,JS::Value *,JS::MutableHandle<JS::Value>) [js/src/jit/BaselineIC.cpp:3890]
#21: ??? (???:???)

At a first glance it seems that it may have been triggered by a call to something like this.textContent = "".

Flags: needinfo?(emilio)
Assignee

Comment 7

Last month

I tried converting the component to not clear out its children and instead set them in the connectedCallback() and modify their hidden state. Even with no changes to the children other than the hidden property this error was occurring.

However, updating the line I linked above where the slot is set on the <named-deck>'s children to always set view.slot = "" avoids the bug and the tests pass (nothing is checking visibility and they're calling click() directly).

Flags: needinfo?(mstriemer)

(In reply to Mark Striemer [:mstriemer] from comment #7)

I tried converting the component to not clear out its children and instead set them in the connectedCallback() and modify their hidden state. Even with no changes to the children other than the hidden property this error was occurring.

However, updating the line I linked above where the slot is set on the <named-deck>'s children to always set view.slot = "" avoids the bug and the tests pass (nothing is checking visibility and they're calling click() directly).

yeah, I digged into it a bit and it definitely seems that the assertion failure is triggered by removing the slot element from the named-deck's shadowRoot, and it doesn't matter how it is removed (e.g. I tried to remove it by calling its remove method instead, right before the AddonCard clears its own content with this.textContent = "", and the stack trace changes accordingly but it is still triggering the assertion failure).

Another odd detail is that the slot element is not actually removed in response of that click, but it is actually triggered way later in the test, when the update is installed (from line 353 of the new version of the browser_html_updates.js, where we call await installUpdate(card, "update-installed");), and in response to that the AddonCard re-renders itself because its onInstallEnded is called for the installed update and this.setAddon(addon) is called.

Comment 9

Last month
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7d165b8966e
Show release notes on HTML about:addons details r=aswan,flod,rpl,kmag

Comment 10

Last month
bugherder
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Thanks, that assertion is much more useful indeed. In hindsight the fact that the assertion could be broken with Shadow DOM seems pretty obvious.

The bug Mark filed has a reduced test-case and a patch.

Flags: needinfo?(emilio)

Updated

Last month
Depends on: 1552447

While validating the fix, I've encountered issue https://bugzilla.mozilla.org/show_bug.cgi?id=1552447 , affecting the Release Notes tab.

You need to log in before you can comment on or make changes to this bug.