Closed Bug 1523194 Opened 5 years ago Closed 5 years ago

Remove XPIDL for DOMLocalization and use do_ImportModule instead

Categories

(Core :: Internationalization, enhancement, P3)

Product:
Core
Component:
Internationalization
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Bug 1523194 - Remove XPIDL for DOMLocalization and use do_ImportModule instead. r?jfkthame,kmag
47 bytes, text/x-phabricator-request
Details | Review

In bug 1513366 kmag introduced do_ImportModule which allows us to drop an XPIDL wrapper.

Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Priority: -- → P3

Kris, I'm trying to use your do_ImportModule to remove a piece of boilerplate and I ran into a weird crash that I don't understand. Can you help me?

What's happening is that after I apply the patch, run a debug build and open about:support (which uses Fluent), I get a crash with the following tract:

#0  0x00007fe3792dc6a8 in nanosleep () at /usr/lib/libc.so.6
#1  0x00007fe3792dc5ae in sleep () at /usr/lib/libc.so.6
#2  0x00007fe373036fd2 in ah_crap_handler(int) (signum=11)
    at /home/zbraniecki/projects/mozilla-unified/toolkit/xre/nsSigHandlers.cpp:95
#3  0x00007fe37301d5eb in nsProfileLock::FatalSignalHandler(int, siginfo_t*, void*)
    (signo=11, info=0x7ffebe2c4eb0, context=0x7ffebe2c4d80)
    at /home/zbraniecki/projects/mozilla-unified/toolkit/profile/nsProfileLock.cpp:174
#4  0x00007fe373cbc7ca in WasmTrapHandler(int, siginfo_t*, void*) (signum=11, info=0x7ffebe2c4eb0, context=0x7ffebe2c4d80)
    at /home/zbraniecki/projects/mozilla-unified/js/src/wasm/WasmSignalHandlers.cpp:928
#5  0x00007fe37971d3c0 in <signal handler called> () at /usr/lib/libpthread.so.0
#6  0x00007fe36e4ba1e3 in RefPtr<mozilla::dom::Promise>::operator->() const (this=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-dbg/dist/include/mozilla/RefPtr.h:295
#7  0x00007fe36e4ba002 in mozilla::dom::DocumentL10n::TriggerInitialDocumentTranslation() (this=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/intl/l10n/DocumentL10n.cpp:343
#8  0x00007fe371514db1 in nsXMLContentSink::HandleEndElement(char16_t const*, bool)
    (this=<optimized out>, aName=<optimized out>, aInterruptable=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/dom/xml/nsXMLContentSink.cpp:1076
#9  0x00007fe36f2dfe1f in nsExpatDriver::HandleEndElement(char16_t const*)
    (this=0x7fe35a88bcc0, aValue=0x7fe35a866e40 u"http://www.w3.org/1999/xhtml\xffffhtml")
    at /home/zbraniecki/projects/mozilla-unified/parser/htmlparser/nsExpatDriver.cpp:300
#10 0x00007fe3720d85f6 in doContent
    (parser=0x7fe35a88f800, startTagLevel=0, enc=0x7fe3752a9150 <little2_encoding_ns>, s=0x7fe359511c46 "<", end=0x7fe359511c56 '\344' <repeats 199 times>, <incomplete sequence \344>..., nextPtr=0x7fe35a88f830, haveMore=1 '\001')
    at /home/zbraniecki/projects/mozilla-unified/parser/expat/lib/xmlparse.c:2964
#11 0x00007fe3720d69e8 in contentProcessor
    (parser=0x7fe35a88f800, start=0xb40 <error: Cannot access memory at address 0xb40>, end=0x7fe3793d4710 <_IO_stdfile_2_lock> "", endPtr=0x7fe37920f740) at /home/zbraniecki/projects/mozilla-unified/parser/expat/lib/xmlparse.c:2514
#12 0x00007fe3720d3eaa in MOZ_XML_ResumeParser (parser=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/parser/expat/lib/xmlparse.c:2155
#13 0x00007fe36f2e1c3f in nsExpatDriver::ParseBuffer(char16_t const*, unsigned int, bool, unsigned int*)
    (this=0x7fe35a88bcc0, aBuffer=0x0, aLength=0, aIsFinal=true, aConsumed=0x7ffebe2c56f4)
    at /home/zbraniecki/projects/mozilla-unified/parser/htmlparser/nsExpatDriver.cpp:794
#14 0x00007fe36f2e2263 in nsExpatDriver::ConsumeToken(nsScanner&, bool&) (this=0x7fe35a88bcc0, 
    aScanner=..., aFlushTokens=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/parser/htmlparser/nsExpatDriver.cpp:891
#15 0x00007fe36f2e5b81 in nsParser::Tokenize(bool) (this=0x7fe35a31a980, aIsFinalChunk=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/parser/htmlparser/nsParser.cpp:1413
#16 0x00007fe36f2e4e27 in nsParser::ResumeParse(bool, bool, bool)
    (this=<optimized out>, allowIteration=true, aIsFinalChunk=<optimized out>, aCanInterrupt=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/parser/htmlparser/nsParser.cpp:963
#17 0x00007fe36f2e4cb9 in nsParser::ContinueInterruptedParsing() (this=0x7fe35a31a980)
    at /home/zbraniecki/projects/mozilla-unified/parser/htmlparser/nsParser.cpp:601
#18 0x00007fe37151a32c in mozilla::detail::RunnableMethodImpl<nsXMLContentSink*, void (nsXMLContentSink::*)(), true, (mozilla::RunnableKind)0>::Run() (this=0x7fe35957f100)
    at /home/zbraniecki/projects/mozilla-unified/obj-x86_64-pc-linux-gnu-dbg/dist/include/nsThreadUtils.h:1171
#19 0x00007fe36e420021 in nsThread::ProcessNextEvent(bool, bool*)
    (this=0x7fe366db6e80, aMayWait=<optimized out>, aResult=0x7ffebe2c5e87)
    at /home/zbraniecki/projects/mozilla-unified/xpcom/threads/nsThread.cpp:1161
#20 0x00007fe36e422265 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7fe366db6e80, aMayWait=false)
    at /home/zbraniecki/projects/mozilla-unified/xpcom/threads/nsThreadUtils.cpp:474
#21 0x00007fe36eb0b039 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)
    (this=<optimized out>, aDelegate=<optimized out>) at /home/zbraniecki/projects/mozilla-unified/ipc/glue/MessagePump.cpp:88
#22 0x00007fe36ea84891 in MessageLoop::RunInternal() (this=0x7fe366dda3e0)
    at /home/zbraniecki/projects/mozilla-unified/ipc/chromium/src/base/message_loop.cc:315
#23 0x00007fe36ea847f2 in MessageLoop::Run() (this=0x7fe366dda3e0)
    at /home/zbraniecki/projects/mozilla-unified/ipc/chromium/src/base/message_loop.cc:290
#24 0x00007fe371796f79 in nsBaseAppShell::Run() (this=0x7fe3662477b0)
    at /home/zbraniecki/projects/mozilla-unified/widget/nsBaseAppShell.cpp:137
#25 0x00007fe372f22a38 in nsAppStartup::Run() (this=0x7fe366db9880)
    at /home/zbraniecki/projects/mozilla-unified/toolkit/components/startup/nsAppStartup.cpp:271
#26 0x00007fe37302db0e in XREMain::XRE_mainRun() (this=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/toolkit/xre/nsAppRunner.cpp:4392
#27 0x00007fe37302e800 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)
    (this=0x7ffebe2c61e0, argc=<optimized out>, argv=<optimized out>, aConfig=...)
    at /home/zbraniecki/projects/mozilla-unified/toolkit/xre/nsAppRunner.cpp:4530
#28 0x00007fe37302ef46 in XRE_main(int, char**, mozilla::BootstrapConfig const&) (argc=2034059024, argv=0x0, aConfig=...)
    at /home/zbraniecki/projects/mozilla-unified/toolkit/xre/nsAppRunner.cpp:4614
#29 0x000055e5a794151d in do_main(int, char**, char**) (argc=4, argv=0x7ffebe2c7528, envp=<optimized out>)
    at /home/zbraniecki/projects/mozilla-unified/browser/app/nsBrowserApp.cpp:214
#30 0x000055e5a794124a in main(int, char**, char**) (argc=4, argv=0x7ffebe2c7528, envp=0x7ffebe2c7550)
    at /home/zbraniecki/projects/mozilla-unified/browser/app/nsBrowserApp.cpp:293

which leads me to: https://searchfox.org/mozilla-central/rev/5c8ea961d04767db723a0a15e3a8f7fbca154129/intl/l10n/DocumentL10n.cpp#342

Do you know why it might crash and is it a limitation od do_ImportModule, a bug in it, or something unrelated?

Flags: needinfo?(kmaglione+bmo)

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)

which leads me to: https://searchfox.org/mozilla-central/rev/5c8ea961d04767db723a0a15e3a8f7fbca154129/intl/l10n/DocumentL10n.cpp#342

Do you know why it might crash and is it a limitation od do_ImportModule, a bug in it, or something unrelated?

DOMLocalization.jsm doesn't export a symbol called translateRoots, so the call returns NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED, and the promise out param stays null.

Presumably you want to define an IDL for the JSM's exports with a DOMLocalization property with the appropriate interface.

Flags: needinfo?(kmaglione+bmo)

Side note: You really need to check for an error after that call. If the JS side throws or returns null for some reason, we really don't want to crash if we don't have to...

DOMLocalization.jsm doesn't export a symbol called translateRoots

DOMLocalization.jsm exports DOMLocalization class which has translateRoots on it. How should I modify the call to use do_ImportModule without the js/manifest boilerplate?

(yeah, I'll fix the check after, thanks!)

Flags: needinfo?(kmaglione+bmo)

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)

DOMLocalization.jsm doesn't export a symbol called translateRoots

DOMLocalization.jsm exports DOMLocalization class which has translateRoots on it. How should I modify the call to use do_ImportModule without the js/manifest boilerplate?

See:

https://searchfox.org/mozilla-central/rev/5c8ea961d04767db723a0a15e3a8f7fbca154129/toolkit/components/extensions/mozIExtensionProcessScript.idl#23-27
https://searchfox.org/mozilla-central/rev/5c8ea961d04767db723a0a15e3a8f7fbca154129/toolkit/components/extensions/ExtensionPolicyService.cpp#62-67

Flags: needinfo?(kmaglione+bmo)

Thanks. I applied the changes into my patchset and it builds properly (updated in phabricator).

I tested rv out of the do_ImportModule, and out of jsm->GetDOMLocalization(getter_AddRefs(mDOMLocalization)); and they both seem to be successes.
Unfortunately, calling any method on mDOMLocalization fails.

How can I get any debug info about why?

Flags: needinfo?(kmaglione+bmo)

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)

I tested rv out of the do_ImportModule, and out of jsm->GetDOMLocalization(getter_AddRefs(mDOMLocalization)); and they both seem to be successes.
Unfortunately, calling any method on mDOMLocalization fails.

How can I get any debug info about why?

Not easily. You could do a debug build, which might give you some messages about failed NS_ENSURE_SUCCESS checks. But, unfortunately, that's become less useful since there was a push a while back to stop using NS_ENSURE_SUCCESS...

In any case, this is failing because you're treating the returned DOMLocalization object as if it were a singleton, when it is in fact a class constructor.

You're going to need to create an instance of it, either from the JS side or the C++ side, and use that instance instead.

Flags: needinfo?(kmaglione+bmo)

I'm going to stop for now. I'm not sure anymore if it really is a cleanup since we have to jump through quite some hoops to get it working. Maybe one day we'll get do_CreateInstance that works with JSM?

Status: ASSIGNED → NEW

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)

I'm going to stop for now. I'm not sure anymore if it really is a cleanup since we have to jump through quite some hoops to get it working. Maybe one day we'll get do_CreateInstance that works with JSM?

You're making this out to be way more complicated than it is. do_ImportModule has exactly the same semantics as a JSM import from JS. You just need to define your exports appropriately. Here:

https://gist.github.com/f6860c04fa4e445a9f4a6c78cabfb1e7

Or if you really want to create a new instance, which I suppose you do:

https://gist.github.com/12a9e443609cd44b809df42804b0d27b

Ahh, yeah. I needed the latter. I now have it working! Thank you!.

Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1030828f04367f64545c33ea6a5ea8103d84140

Status: NEW → ASSIGNED
Attachment #9039431 - Attachment description: Bug 1523194 - Remove XPIDL for DOMLocalization and use do_ImportModule instead. r?jfkthame → Bug 1523194 - Remove XPIDL for DOMLocalization and use do_ImportModule instead. r?jfkthame,kmag
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/270345fe62b9
Remove XPIDL for DOMLocalization and use do_ImportModule instead. r=jfkthame,kmag

https://hg.mozilla.org/mozilla-central/rev/270345fe62b9

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2a0a5ce8fdea
Port bug 1523194 - Remove mozDOMLocalization.js/manifest from package manifest. rs=bustage-fix
Regressions: 1550951
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: