Closed Bug 1444285 Opened 2 years ago Closed 2 years ago

"Assertion failure: !aReaction->IsUpgradeReaction() (Upgrade reaction should not be scheduled to backup queue)" when using a XUL Custom Element

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bgrins, Assigned: smaug)

References

Details

Attachments

(3 files)

In a try push on the latest patch from Bug 1411707, I see assertion failures on some of the bc suites:

GECKO(1835) | Assertion failure: !aReaction->IsUpgradeReaction() (Upgrade reaction should not be scheduled to backup queue), at /builds/worker/workspace/build/src/dom/base/CustomElementRegistry.cpp:1065

For example, from https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7442d4f9ff4026196709b7fe2b923acca865ec2&selectedJob=166895715.
This looks like the same thing from https://bugzilla.mozilla.org/show_bug.cgi?id=1404420#c40:

> Upgrade reactions should not be scheduled to backup queue per spec, so we
> will hit this assertion if we miss AutoCEReaction on the codepath which will
> trigger enqueueing an upgrade reaction.
> 
> In the crash case, the upgrade reaction is from
> http://searchfox.org/mozilla-central/rev/
> 40e8eb46609dcb8780764774ec550afff1eed3a5/dom/xbl/nsXBLService.cpp#506. We
> probably could add AutoCEReaction (Before nsAutoScriptBlocker) if the
> enqueueing upgrade reaction is expected.
I can reproduce this locally with the patch applied using: `./mach mochitest browser/base/content/test/alerts/browser_notification_open_settings.js`
Blocks: war-on-xbl
This appears to happen when the Custom Element (a stringbundle, in this case) is created inside of XBL anonymous content.

Removing this tag causes the assertion to go away: https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/toolkit/content/widgets/filefield.xml#17
The patch fixes most of the crashes but is now failing accessibility and some other tests with "Assertion failure: !mMightHaveUnreportedJSException": https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fa82e6810b012e9bcce9cb9b61dd4a8c93da650&selectedJob=167121012
Looks like treeherder doesn't have access to the logs atm.
...since the stack trace for those assertion failures would be interesting to know.
The patch didn't seem to compile. Made a tiny tweak and pushed to try again.

remote: View the pushlog for these changes here:
remote:   https://hg.mozilla.org/try/pushloghtml?changeset=7c003ddb835fc0a2b13a584efd1ac07f3b305d19
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c003ddb835fc0a2b13a584efd1ac07f3b305d19
remote: recorded changegroup in replication log in 0.073s
That tryserver build doesn't show the a11y issue. Perhaps the tree was in bad state when you pushed the patch.
But I retriggered a11y tests.
Or, hmm, should I have included some other stuff with the try push?
Flags: needinfo?(bgrinstead)
(In reply to Olli Pettay [:smaug] from comment #10)
> Or, hmm, should I have included some other stuff with the try push?

Yes there needs to be a Custom Element registered that gets used inside of XBL anonymous content. The original try push had one, but it's now bitrotted. I'll push up a patch that does it.
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #11)
> (In reply to Olli Pettay [:smaug] from comment #10)
> > Or, hmm, should I have included some other stuff with the try push?
> 
> Yes there needs to be a Custom Element registered that gets used inside of
> XBL anonymous content. The original try push had one, but it's now
> bitrotted. I'll push up a patch that does it.

If you apply the rev from this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bf324e5a694ad5f5ee42b8b18d0e2edc657697b (specifically https://hg.mozilla.org/try/rev/b68e1a5a3c1e6d13a32718f4cec1a6f775067bba), then do `./mach test  browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js` you should hit the original error:

GECKO(37825) Assertion failure: !aReaction->IsUpgradeReaction() (Upgrade reaction should not be scheduled to backup queue), at /builds/worker/workspace/build/src/dom/base/CustomElementRegistry.cpp:1188

Then with your unbitrotted version of the patch from this bug, I think that particular test should be fixed but other tests will hit the new `Assertion failure: !mMightHaveUnreportedJSException` error.
Pushed up the folded together version from the try push in Comment 8
Here's a try push showing the failure with the patches applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3624f29f48aa20396f96b18c493591785b431fed&selectedJob=175155532
Flags: needinfo?(bugs)
Copy/pasting the stack since we lost the last try push:

[task 2018-04-23T18:37:41.993Z] 18:37:41     INFO - GECKO(1041) | Assertion failure: !mMightHaveUnreportedJSException, at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ErrorResult.h:487
[task 2018-04-23T18:38:08.845Z] 18:38:08     INFO - GECKO(1041) | #01: mozilla::binding_danger::TErrorResult<mozilla::binding_danger::AssertAndSuppressCleanupPolicy>::AssertReportedOrSuppressed [dom/bindings/ErrorResult.h:487]
[task 2018-04-23T18:38:08.847Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.849Z] 18:38:08     INFO - GECKO(1041) | #02: mozilla::binding_danger::TErrorResult<mozilla::binding_danger::AssertAndSuppressCleanupPolicy>::~TErrorResult [dom/bindings/ErrorResult.h:143]
[task 2018-04-23T18:38:08.850Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.852Z] 18:38:08     INFO - GECKO(1041) | #03: mozilla::dom::CustomElementReactionsStack::InvokeReactions [dom/bindings/ErrorResult.h:555]
[task 2018-04-23T18:38:08.853Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.855Z] 18:38:08     INFO - GECKO(1041) | #04: mozilla::dom::CustomElementReactionsStack::PopAndInvokeElementQueue [dom/base/CustomElementRegistry.cpp:1136]
[task 2018-04-23T18:38:08.855Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.857Z] 18:38:08     INFO - GECKO(1041) | #05: mozilla::Maybe<mozilla::dom::AutoCEReaction>::reset [mfbt/Maybe.h:535]
[task 2018-04-23T18:38:08.857Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.862Z] 18:38:08     INFO - GECKO(1041) | #06: nsXBLService::LoadBindings [dom/xbl/nsXBLService.cpp:581]
[task 2018-04-23T18:38:08.862Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.864Z] 18:38:08     INFO - GECKO(1041) | #07: nsCSSFrameConstructor::AddFrameConstructionItemsInternal [layout/base/nsCSSFrameConstructor.cpp:5677]
[task 2018-04-23T18:38:08.864Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.866Z] 18:38:08     INFO - GECKO(1041) | #08: nsCSSFrameConstructor::DoAddFrameConstructionItems [layout/base/nsCSSFrameConstructor.cpp:5550]
[task 2018-04-23T18:38:08.868Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.869Z] 18:38:08     INFO - GECKO(1041) | #09: nsCSSFrameConstructor::AddFrameConstructionItems [layout/base/nsCSSFrameConstructor.cpp:5563]
[task 2018-04-23T18:38:08.871Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.873Z] 18:38:08     INFO - GECKO(1041) | #10: nsCSSFrameConstructor::ProcessChildren [layout/base/nsCSSFrameConstructor.cpp:10283]
[task 2018-04-23T18:38:08.873Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.876Z] 18:38:08     INFO - GECKO(1041) | #11: nsCSSFrameConstructor::ConstructFrameFromItemInternal [layout/base/nsCSSFrameConstructor.cpp:4037]
[task 2018-04-23T18:38:08.877Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.879Z] 18:38:08     INFO - GECKO(1041) | #12: nsCSSFrameConstructor::ConstructFramesFromItem [layout/base/nsCSSFrameConstructor.cpp:6067]
[task 2018-04-23T18:38:08.880Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.882Z] 18:38:08     INFO - GECKO(1041) | #13: nsCSSFrameConstructor::ConstructFramesFromItemList [layout/base/nsCSSFrameConstructor.cpp:10104]
[task 2018-04-23T18:38:08.882Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.884Z] 18:38:08     INFO - GECKO(1041) | #14: nsCSSFrameConstructor::ProcessChildren [layout/base/nsCSSFrameConstructor.cpp:10304]
[task 2018-04-23T18:38:08.885Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.887Z] 18:38:08     INFO - GECKO(1041) | #15: nsCSSFrameConstructor::ConstructFrameFromItemInternal [layout/base/nsCSSFrameConstructor.cpp:4037]
[task 2018-04-23T18:38:08.887Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.889Z] 18:38:08     INFO - GECKO(1041) | #16: nsCSSFrameConstructor::ConstructFramesFromItem [layout/base/nsCSSFrameConstructor.cpp:6067]
[task 2018-04-23T18:38:08.889Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.892Z] 18:38:08     INFO - GECKO(1041) | #17: nsCSSFrameConstructor::ConstructFramesFromItemList [layout/base/nsCSSFrameConstructor.cpp:10104]
[task 2018-04-23T18:38:08.892Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.894Z] 18:38:08     INFO - GECKO(1041) | #18: nsCSSFrameConstructor::ProcessChildren [layout/base/nsCSSFrameConstructor.cpp:10304]
[task 2018-04-23T18:38:08.895Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.897Z] 18:38:08     INFO - GECKO(1041) | #19: nsCSSFrameConstructor::ConstructDocElementFrame [layout/base/nsCSSFrameConstructor.cpp:2675]
[task 2018-04-23T18:38:08.898Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.900Z] 18:38:08     INFO - GECKO(1041) | #20: nsCSSFrameConstructor::ContentRangeInserted [layout/base/nsCSSFrameConstructor.cpp:7470]
[task 2018-04-23T18:38:08.901Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.902Z] 18:38:08     INFO - GECKO(1041) | #21: nsCSSFrameConstructor::ContentInserted [layout/base/nsCSSFrameConstructor.cpp:7366]
[task 2018-04-23T18:38:08.903Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.905Z] 18:38:08     INFO - GECKO(1041) | #22: mozilla::PresShell::Initialize [layout/base/PresShell.cpp:1822]
[task 2018-04-23T18:38:08.906Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.908Z] 18:38:08     INFO - GECKO(1041) | #23: mozilla::dom::XULDocument::StartLayout [dom/xul/XULDocument.cpp:1617]
[task 2018-04-23T18:38:08.909Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.911Z] 18:38:08     INFO - GECKO(1041) | #24: mozilla::dom::XULDocument::DoneWalking [dom/xul/XULDocument.cpp:2793]
[task 2018-04-23T18:38:08.912Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.913Z] 18:38:08     INFO - GECKO(1041) | #25: mozilla::dom::XULDocument::ResumeWalk [dom/xul/XULDocument.cpp:2719]
[task 2018-04-23T18:38:08.914Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.916Z] 18:38:08     INFO - GECKO(1041) | #26: mozilla::dom::XULDocument::OnScriptCompileComplete [dom/xul/XULDocument.cpp:3233]
[task 2018-04-23T18:38:08.916Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.917Z] 18:38:08     INFO - GECKO(1041) | #27: mozilla::dom::XULDocument::OnStreamComplete [dom/xul/XULDocument.cpp:3148]
[task 2018-04-23T18:38:08.917Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.919Z] 18:38:08     INFO - GECKO(1041) | #28: mozilla::net::nsStreamLoader::OnStopRequest [netwerk/base/nsStreamLoader.cpp:110]
[task 2018-04-23T18:38:08.919Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.920Z] 18:38:08     INFO - GECKO(1041) | #29: nsBaseChannel::OnStopRequest [netwerk/base/nsBaseChannel.cpp:878]
[task 2018-04-23T18:38:08.922Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.923Z] 18:38:08     INFO - GECKO(1041) | #30: nsInputStreamPump::OnStateStop [netwerk/base/nsInputStreamPump.cpp:707]
[task 2018-04-23T18:38:08.923Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.925Z] 18:38:08     INFO - GECKO(1041) | #31: nsInputStreamPump::OnInputStreamReady [netwerk/base/nsInputStreamPump.cpp:444]
[task 2018-04-23T18:38:08.926Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.927Z] 18:38:08     INFO - GECKO(1041) | #32: nsInputStreamReadyEvent::Run [xpcom/io/nsStreamUtils.cpp:102]
[task 2018-04-23T18:38:08.928Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.929Z] 18:38:08     INFO - GECKO(1041) | #33: nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:1100]
[task 2018-04-23T18:38:08.930Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.932Z] 18:38:08     INFO - GECKO(1041) | #34: NS_ProcessNextEvent [xpcom/threads/nsThreadUtils.cpp:519]
[task 2018-04-23T18:38:08.933Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.934Z] 18:38:08     INFO - GECKO(1041) | #35: mozilla::ipc::MessagePump::Run [ipc/glue/MessagePump.cpp:98]
[task 2018-04-23T18:38:08.934Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.936Z] 18:38:08     INFO - GECKO(1041) | #36: MessageLoop::RunInternal [ipc/chromium/src/base/message_loop.cc:327]
[task 2018-04-23T18:38:08.936Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.938Z] 18:38:08     INFO - GECKO(1041) | #37: MessageLoop::Run [ipc/chromium/src/base/message_loop.cc:298]
[task 2018-04-23T18:38:08.939Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.941Z] 18:38:08     INFO - GECKO(1041) | #38: nsBaseAppShell::Run [widget/nsBaseAppShell.cpp:159]
[task 2018-04-23T18:38:08.942Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.943Z] 18:38:08     INFO - GECKO(1041) | #39: nsAppStartup::Run [toolkit/components/startup/nsAppStartup.cpp:291]
[task 2018-04-23T18:38:08.944Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.945Z] 18:38:08     INFO - GECKO(1041) | #40: XREMain::XRE_mainRun [toolkit/xre/nsAppRunner.cpp:4835]
[task 2018-04-23T18:38:08.947Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.947Z] 18:38:08     INFO - GECKO(1041) | #41: XREMain::XRE_main [toolkit/xre/nsAppRunner.cpp:4979]
[task 2018-04-23T18:38:08.947Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.948Z] 18:38:08     INFO - GECKO(1041) | #42: XRE_main [toolkit/xre/nsAppRunner.cpp:5072]
[task 2018-04-23T18:38:08.948Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.984Z] 18:38:08     INFO - GECKO(1041) | #43: do_main [browser/app/nsBrowserApp.cpp:231]
[task 2018-04-23T18:38:08.984Z] 18:38:08     INFO - 
[task 2018-04-23T18:38:08.986Z] 18:38:08     INFO - GECKO(1041) | #44: main [browser/app/nsBrowserApp.cpp:306]
Ah, that is interesting and worrisome stack. We can't invoke reactions in that unsafe place.
And the assertion happens because of https://searchfox.org/mozilla-central/rev/fcd5cb3515d0e06592bba42e378f1b9518497e3d/dom/base/CustomElementRegistry.cpp#1209
Flags: needinfo?(bugs)
I wonder... perhaps we could just disable the assertion on system principal documents.
The assertion was added because per spec upgrade shouldn't in practice ever use backup queue, but
the spec doesn't know about XBL.

Need to still fix https://searchfox.org/mozilla-central/rev/fcd5cb3515d0e06592bba42e378f1b9518497e3d/dom/base/CustomElementRegistry.cpp#1209 though.
Attached patch ce_in_xbl.diffSplinter Review
At least (debug) browser starts with this :)

But anyway, this might not be too bad.


remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/7a6fbbf3b1e50d91c056ed55e07a44602399ffd4
remote:   https://hg.mozilla.org/try/rev/f702dbc8138e3895f5519bfb670fc6d135784772
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=f702dbc8138e3895f5519bfb670fc6d135784772
remote: recorded changegroup in replication log in 0.114s
(In reply to Olli Pettay [:smaug] from comment #19)
> Created attachment 8970603 [details] [diff] [review]
> ce_in_xbl.diff
> 
> At least (debug) browser starts with this :)
> 
> But anyway, this might not be too bad.
> 
> 
> remote: View your changes here:
> remote:  
> https://hg.mozilla.org/try/rev/7a6fbbf3b1e50d91c056ed55e07a44602399ffd4
> remote:  
> https://hg.mozilla.org/try/rev/f702dbc8138e3895f5519bfb670fc6d135784772
> remote: 
> remote: Follow the progress of your build on Treeherder:
> remote:  
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f702dbc8138e3895f5519bfb670fc6d135784772
> remote: recorded changegroup in replication log in 0.114s

a11y and bc tests look good on that push!
Comment on attachment 8970603 [details] [diff] [review]
ce_in_xbl.diff

mrbkap, this makes us sort of fallback to backupqueue even for upgrades in case custom element is inside xbl. So when we're instantiating XBL, we need to queue upgrades. Relaxed the assertions only for chrome code, since XBL with CE should run only there and for the web upgrades shouldn't happen using backup queue (only other reactions).
It is possible we will need to tweak this while converting more XBL stuff to custom elements, but if we could live with this rather small change, I'd prefer that.
Backup queue scheduling should ensure constructor runs at safe time during microtask checkpoint.
Attachment #8970603 - Flags: review?(mrbkap)
Assignee: nobody → bugs
Priority: -- → P2
Comment on attachment 8970603 [details] [diff] [review]
ce_in_xbl.diff

Review of attachment 8970603 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementRegistry.cpp
@@ +1238,5 @@
>  
>      auto& reactions = elementData->mReactionQueue;
>      for (uint32_t j = 0; j < reactions.Length(); ++j) {
>        // Transfer the ownership of the entry due to reentrant invocation of
>        // this funciton. The entry will be removed when bug 1379573 is landed.

While reading code and bugs, I noticed that this bug is now WONTFIX. Might as well remove the comment referring to it.
Attachment #8970603 - Flags: review?(mrbkap) → review+
Attached patch +comment fixSplinter Review
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6825c9cd5bd5
let custom element upgrades to use backup queue inside chrome/XBL, r=mrbkap
https://hg.mozilla.org/mozilla-central/rev/6825c9cd5bd5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.